Codify style rules

[ryan-williams]

I recently removed Scalariform due to conflicts between it, my personal preferences, and (most importantly) IntelliJ that I wasn't able to resolve.

In #435, #436, and #437, some questions have been raised about what styles we should enforce, disallow, allow existing violations of, and even allow new deviations from.

I'm personally fine with a maximally hands-off posture where:

• we agree on preferred style patterns,
• changes that improve code according to that hierarchy are welcomed,
• existing and new deviations are not required to be fixed.

In all cases, the ability of IntelliJ to format code according to a style automatically is a strong argument in favor of a given style.

To seed discussion, here's the ScalaDoc tab of my IntelliJ Code Style > Scala preferences:

Let me know if you have something different and feel we should standardize on that.

[ryan-williams]

I bias towards complying with IntelliJ's style warnings, but one that I'd not really thought about is .size vs. .length on Strings and Arrays; the "more info" makes a reasonably compelling case, though:

[arahuja]

For those that might be interested (likely no one else) scalariform does not need to run through maven but can also run as a command line tool: https://github.com/scala-ide/scalariform/

Some properties of use that seem to come up may be:
alignSingleLineCaseStatements to true
placeScaladocAsterisksBeneathSecondAsterisk to false (depending on result of #469 )
newlineAtEndOfFile to true
danglingCloseParenthesis to Force
spaceInsideBrackets to false
spacesAroundMultiImports to false (I think this was the original property most did not like from scalariform)

[ryan-williams]

Good finds @arahuja. I noticed this possible scalastyle integration in IntelliJ as well:

which I've seen in some other projects, I think, and could be worth investigating.

[ryan-williams]

Per discussion on #475, I prefer avoiding wildcard imports, the most common exception being when importing implicits from an object.

IntelliJ makes this easy to automate by setting a high number here:

[ryan-williams]

This just came up on #569:

Here are four* alternatives for doing one thing (bar, which takes a Foo) or another (baz) based on an option being Some or None, resp.:

match

fooOpt match {
  case Some(foo) => bar(foo)
  case None => baz
}

fold

fooOpt.fold(baz)(
  foo => bar(foo)
)

map+getOrElse

fooOpt
  .map(foo => bar(foo))
  .getOrElse(baz)

if/else + .*Empty/.get

if (fooOpt.nonEmpty) {
  val foo = fooOpt.get
  bar(foo)
} else {
  baz
}

I really feel like the match is cleanest; the others are repurposing primitives that are built for other purposes.

@arahuja you prefer the fold?

*(updated to add a fourth case and make the hypothetical bar action take a Foo param)

[arahuja]

The fold and map+getOrElse are not necessarily interchangeable. I think I'd prefer map+getOrElse in most cases, but in the case referenced at #569, it would required a chainable .map.

the others are repurposing primitives that are built for other purposes.

I don't think this is true - an Option is a container just as any another? Ocaml Option has .map as well: http://ocaml-lib.sourceforge.net/doc/Option.html as does @smondet's nonstd https://bitbucket.org/smondet/nonstd/src/203b3f367f7236fb341998165a39e1e4edfdd53c/nonstd.ml?at=master&fileviewer=file-view-default#nonstd.ml-386:393

[ryan-williams]

The fold and map+getOrElse are not necessarily interchangeable.

Do you have an example? The Option.fold docs say:

@note This is equivalent to `$option map f getOrElse ifEmpty`.

I think I'd prefer map+getOrElse in most cases

I feel pretty confident in saying this is not what we should do/recommend.

Option.map is useful if you are e.g. chaining one or more transformations onto the contained value, and want to stay lifted in Option-space.

Using .map(…).getOrElse(…) to do what we want on #569 (one thing if full, another if empty) is quite indirect: why bother creating the Option that .map returns, only to immediately open it back up and discard it?

Furthermore, .map and .getOrElse also both strongly imply that the returned values are important, which is not the case on #569, as bar and baz are both only wanted for their side-effects.

…but in the case referenced at #569, it would required a changeable .map

Can you elaborate? I'm not sure what you mean by "a changeable .map".

If I filled in bar and baz appropriately in my previous examples (note that I added a 4th, using if/else+.nonEmpty/.get), each could do exactly what you did in #569, no?

an Option is a container just as any another?

If you mean it's like Iterable et al, which also have map/fold/foreach, then the answer is "sort of":

• it doesn't inherit from any of them,
• but it does have an always-available implicit conversion to Iterable.

Of course, Option also exposes Option-specific APIs beyond its Iterable-like APIs, like exhaustive-matching its two sealed cases.

The logic in #569 (do one thing if the option is full, another if it's empty) has no analog on a collection-monad, so there's no reason to prefer using the Option's [collection-of-size-1]-style APIs over its [full-xor-empty] APIs; just the opposite, in fact.

Option has .map as well

Sure… that doesn't mean that .map is the best way to express any arbitrary thing, especially when it has more specific APIs that more directly map to what we're doing on #569, per above.

Ocaml Option has .map as well, as does @smondet's nonstd

No one is saying .map is not a useful API to have exist on an Option monad.

More pertinently, OCaml's Option also has Some/None pattern-matching, which is a really good way to handle the case where you want to do one thing with the option's value iff it has one, and another thing if the option is empty, especially when neither of those things need return anything or keep anything in Option-space.

So hopefully it's clear that map+getOrElse is not a good approach for applying exactly one of two Unit impure actions based on the full-ness or emptiness of an Option.

Between folding and matching, we can debate what is clearer;

• Option.fold:
  • Also carries an implication that the return value is important.
  • Seems to me like a carry-over from the collection monad, where it does something meaningfully distinct from other APIs: accumulating and updating state during a traversal, and returning the result.
  • On an option, otoh, you'd have to really squint to interpret some computation (esp. that in Upgrade Hadoop BAM for better interval filtering #569) as a stateful-traversal over a collection-of-size-1.
  • Worse, the default value in the fold is presented up front, but passed "by name", so that it is only evaluated if the option is empty.
    • This stands in contrast to the behavior of collections' fold, which eagerly evaluate the default value.
    • I didn't know about this difference until checking the source, and am curious which behavior you intended on Upgrade Hadoop BAM for better interval filtering #569?
• Option's Some-destructuring/unapply provides a direct way to handle the Option's two, sealed cases.