Adding newlines at end of top level definition

[nasadorian]

Feature request

Hi All! This is a feature request that I'd be interested in working on myself, as a first time contribution. I'd love to hear feedback if anyone else cares about this kind of a rewrite rule.

There currently exists a scalafmt rule which adds a newline after top level statements. For consistency's sake, it would be great to have a symmetric additional rule which adds a newline before the closing brace of a top level module (object or class definition).

Current

Scalafmt currently adds a newline at the beginning of top level statements. For example with the following configuration:

version = 2.0.1
newlines.alwaysBeforeTopLevelStatements = true

Given code like this:

object Foo {
  def someCode: Unit =
    ()
}

Scalafmt produces this:

object Foo {

  def someCode: Unit =
    ()
}

Proposed

For smaller objects particularly, the above rule ends up with asymmetric looking code. So it would be nice to have an optional trailing newline as well.

So given a (hypothetical) configuration like this:

version = 2.0.1
newlines.alwaysBeforeTopLevelStatements = true
newlines.alwaysAfterTopLevelStatements = true // new rule

Given this code:

object Foo {
  def someCode: Unit
}

Scalafmt would produce:

object Foo {

  def someCode: Unit

}

[poslegm]

I found PR with flag introduction #733, but motivation is lost in old gitter discussion. Symmetric code looks reasonable, but I don't think that another configuration flag is good decision.

What about small configuration refactoring? New flag with all cases coverage:

newlines.alwaysInsideTopLevelStatements = [before, after, both, never]

Current flag will be deprecated and kept for compatibility.

@olafurpg @tanishiking @kitbellew

[nasadorian]

@poslegm I think this is a flexible solution. We may want to call the new configuration newlines.insideTopLevelStatements though, since it is not always.

I started poking around in the code last night but I'm not yet familiar with how scalafmt works. Any pointers to get started on this would be appreciated. Maybe for example, where (and how) the current newline rule is implemented?

[poslegm]

You can just search by rule name:

scalafmt/scalafmt-core/shared/src/main/scala/org/scalafmt/internal/FormatWriter.scala

Line 319 in e02b1fe

initStyle.newlines.alwaysBeforeTopLevelStatements && {

But I prefer to wait for the decisions of other maintainers about this change.

[kitbellew]

@nasadorian @poslegm generally, i am in favour (and the "both" version specifically). however, the way our team prefers to implement this rule is not super-straightforward. we consider these newlines as visual "breathing" space to increase readability, and apply it when readability is actually improved.

for instance, there's no need for vertical lines here:

class A(str: String) {
  val length: Int = str.length
  def apply(i: Int): Char = str(i)
}

I think we prefer to surround with empty lines (i.e. "both") any multiline method and any sequence of single-line methods WHEN the class contains at least one empty line. like this:

class A(str: String) {

  val length: Int = str.length
  def apply(i: Int): Char = str(i)

  def validateNonNull(): Unit = {
    if (str == null)
      throw NullPointerException("oy vey")
  }

}

[nasadorian]

@kitbellew I like the idea of keeping small top-level statements tight. Adding lines around something like a typeclass trait introduces unecessary noise. EDIT: At second glance, scalafmt seems to already do this.

However I'm not sure I like the uneven spacing in the 2nd example between length and apply. As a user I would expect the formatting to just be uniform.

class A(str: String) {

  val length: Int = str.length

  def apply(i: Int): Char = str(i)

  def validateNonNull(): Unit = {
    if (str == null)
      throw NullPointerException("oy vey")
  }

}

Is there room for more configurability to serve both styles? Maybe some additional boolean flag with a default like newlines.ignoreContiguousStatements = true?

[poslegm]

I propose to keep this rule as simple as possible (at least at first iteration). We can firstly solve the problem with assymetric formatting and then discuss about more complex logic:

I think we prefer to surround with empty lines (i.e. "both") any multiline method and any sequence of single-line methods WHEN the class contains at least one empty line. like this: