Add trailingCommas option #1174

gabro · 2018-05-11T14:05:17Z

Closes #1173
Closes #1060

Examples, given this config:

trailingCommas = always
align = none
danglingParentheses = true

Before	After
def method( a: String, b: Int )	def method( a: String, b: Int, )
// maxColumn \| def method(a: String, b: Int)	def method( a: String, b: Int, )
// maxColumn \| import x.y.{memberA, memberB}	import x.y.{ memberA, memberB, }
// maxColumn \| def method[Key, Value](paramA: Int, paramB: Int)	// maxColumn \| def method[ Key, Value, ]( paramA: Int, paramB: Int, )

Before moving forward, I would appreciate if @olafurpg could take a quick look, just to see whether I'm doing this right 😅

gabro · 2018-05-11T16:13:50Z

scalafmt-tests/src/test/resources/unit/Dialect.stat

@@ -7,7 +7,7 @@ foo(
 >>>
 foo(
    1,
-    2,
+    2


This PR is currently breaking. I have an idea on how to make this non-breaking by default.

olafurpg

This looks awesome, I'm eager to use this feature myself. The approach looks exactly like what I had in mind 👍

Missing test cases

foo(
  a, // comment
)

and if optIn.configStyleArguments = false

// before
foo(
  a,
)
// after
foo(a)

olafurpg · 2018-05-11T16:13:03Z

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

@@ -43,6 +43,24 @@ class FormatWriter(formatOps: FormatOps) {
                .getOrElse(token.syntax, token.syntax)
            sb.append(rewrittenToken)
        }
+
+        if (runner.dialect.allowTrailingCommas &&
+          state.splits.last.modification.isNewline &&


It would be good to have a parallel that removes the trailing comma if !modification.isNewline

Yep, it may be necessary for handling configStyleArguments = false

also necessary for

foo(a, b, )

yep, I have a skipped test for it. Thanks for the examples, it's hard to come up with those :D

The comma should btw be removed in that case regardless of the setting since it's a bug to produce foo(a, b,)

olafurpg · 2018-05-11T16:15:13Z

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

@@ -43,6 +43,24 @@ class FormatWriter(formatOps: FormatOps) {
                .getOrElse(token.syntax, token.syntax)
            sb.append(rewrittenToken)
        }
+
+        if (runner.dialect.allowTrailingCommas &&


Let's move this to a helper method handleTrailingComma or something like that

I agree, I was about to do it!

olafurpg · 2018-05-11T16:15:52Z

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

+
+        if (runner.dialect.allowTrailingCommas &&
+          state.splits.last.modification.isNewline &&
+          (formatToken.right.is[Token.RightParen] ||


Conventionally we pattern match against case FormatToken(_: Token.Comma, _: Token.RightParen | _: Token.RightBracket, _)

Actually, pattern matching might not be a good idea since we'll have to handle comments as well. This is fine 👍

gabro · 2018-05-11T20:57:00Z

Ok, now it handles the removal of trailing commas when going from multi-line to single line, e.g.

// before
foo(
  a,
)

// after
foo(a)

or

// before
foo(a,
  b,
)

// after
foo(a, b)

Comments are next!

The tests are skipped because of an upstream parser bug in Scalameta. See scalameta/scalameta#1531

gabro · 2018-05-12T09:26:58Z

I've added support for handling comments after trailing commas when adding/removing:

foo(
  a,
  b, // comment
)

Unfortunately I hit scalameta/scalameta#1531 which makes the test fail. I was able to test the addition nonetheless, since the bug affects the output, but I'm not confident the removal works, since the test input doesn't parse.

gabro · 2018-05-12T09:30:10Z

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

+      case TrailingCommas.always
+          if left.is[Comment] && !prev(left).is[Comma] && right.is[ClosingBracket] && isNewline =>
+        sb.insert(sb.length - left.syntax.length - 1, ",")
+        sb.append(whitespace)


I had to deal with comments in this way, detecting a comment, looking for a comma next to it, going back in the buffer and removing the comma.

Not sure how safe is this.

I had initially tried to incorporate this into the previous case, but in presence of [ID][COMMENT][NEWLINE][CLOSING_BRACKET] I couldn't detect the newline after che comment.

gabro · 2018-05-12T09:30:44Z

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

+      case TrailingCommas.never
+          if left.is[Comment] && prev(left).is[Comma] && right.is[ClosingBracket] && isNewline =>
+        sb.deleteCharAt(sb.length - left.syntax.length - 1)
+        sb.append(whitespace)


This is the case that is currently untested, since the test input fails to parse.

gabro · 2018-05-12T10:49:14Z

Ok, I've tried using scalameta/scalameta#1532 with a locally published version of Scalameta, and I've found a couple of bugs with the comments. It seems my approach is not idempotent. I'll investigate!

gabro · 2018-05-13T17:24:33Z

Done! I've used scalameta/scalameta#1532 to fix the two tests that are skipped in this PR, and they all pass on my machine.

gabro · 2018-05-13T17:26:04Z

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

@@ -43,7 +43,9 @@ class FormatWriter(formatOps: FormatOps) {
                .getOrElse(token.syntax, token.syntax)
            sb.append(rewrittenToken)
        }
-        sb.append(whitespace)
+
+        handleTrailingCommasAndWhitespace(formatToken, state, sb, whitespace)


I had to incorporate the insertion of the whitespace, since in one case it needs to be skipped, namely when rewriting

foo(a, b, )

to

foo(a, b)

Inserting the whitespace would result in

foo(a, b )

instead

olafurpg · 2018-05-13T18:10:43Z

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

+      case TrailingCommas.always
+          if !left.is[Comma] && !left.is[Comment] &&
+            right.is[CloseDelim] && isNewline =>
+        sb.append(",")


Does this force a comma in this case?

// before lst.map { x => x + 1 } // after lst.map { x => x + 1, }

I think we may have to limit this behavior to a couple of nodes

Term.Apply

Importee/Importer

Type.Apply

Also tuples, I think. How do I limit to a specific node?

Right, tuples as well 👍

FormatOps.owners(Token): Tree goes to the closest enclosing node of a token

done! The final list includes also Defn.Def and Decl.Def. I've added test cases for all of those.

gabro · 2018-05-14T09:24:08Z

scalafmt-tests/src/test/resources/trailing-commas/trailingCommasAlwaysComments.stat

+maxColumn = 30
+trailingCommas = always
+
+<<< SKIP should consider comments when adding trailing commas


works locally with scalameta master which includes scalameta/scalameta#1532

Should we track to enable this somewhere, @olafurpg?

It may be a couple weeks until the next meta release so I think it would be good to track this in a ticket.

gabro · 2018-05-14T09:24:18Z

scalafmt-tests/src/test/resources/trailing-commas/trailingCommasNeverComments.stat

+maxColumn = 30
+trailingCommas = never
+
+<<< SKIP should consider comments when removing trailing commas


same as above

olafurpg · 2018-05-14T10:50:44Z

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

+
+    val owner = owners(formatToken.right)
+    val hasValidOwner =
+      owner.is[Term.Apply] || owner.is[Term.Tuple] ||


Can we introduce a classifier named CommaSeparated for this group? I think Ctor.{Primary,Secondary} are missing for

class Foo( a: Int, b: String, )

Nice catch, yes I'll introduce a classifier if you think it's easier to read

gabro · 2018-05-14T13:30:20Z

scalafmt-core/shared/src/main/scala/org/scalafmt/util/TreeOps.scala

@@ -318,6 +319,11 @@ object TreeOps {
  def isDefnOrCallSite(tree: Tree): Boolean =


this method was already here, I realized!

gabro · 2018-05-14T13:31:40Z

@olafurpg take a look, I've re-used an existing method in FormatOps, so now the owner check should be fairly comprehensive. I've also added a bunch of new test cases for stuff like

val (
  a,
  b
) = someTuple

olafurpg

LGTM 👍 Feel free to merge once ./scalafmt --diff is happy 😊

olafurpg · 2018-05-14T13:51:28Z

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

+
+      // foo(
+      //   a,
+      //   b, // comment


👏 Super nice comments, nice break down

Those were mostly for myself 😅 They seemed useful so I've left them

olafurpg · 2018-05-14T13:54:48Z

scalafmt-core/shared/src/main/scala/org/scalafmt/config/ScalafmtConfig.scala

@@ -124,6 +134,7 @@ case class ScalafmtConfig(
    assumeStandardLibraryStripMargin: Boolean = false,
    danglingParentheses: Boolean = false,
    poorMansTrailingCommasInConfigStyle: Boolean = false,
+    trailingCommas: TrailingCommas = TrailingCommas.preserve,


I wonder what is the best default 🤔 Personally, I think always might be a good choice since I'd prefer scalafmt to be opinionated where possible. However, that will break code on 2.11.

I'll open a discussion to gauge opinions, until then I agree preserve is the safest choice 👍

I wonder if the default could depend the dialect of the runner

gabro · 2018-05-14T14:15:34Z

CI is 🍏 merging! Thank you so much @olafurpg for the many and quick reviews!

olafurpg

Seems we forgot to document the new setting on the website, I'll write about it in the changelog but I think it would be a good candidate for the "Popular" section

gabro · 2018-05-14T14:21:21Z

Seems we forgot to document the new setting on the website, I'll write about it in the changelog but I think it would be a good candidate for the "Popular" section

Sort of. I didn't forget, but I didn't want to put energy in writing docs before completing the new website (maybe I'm ambitious :P )

olafurpg · 2018-05-14T14:24:28Z

I'm fine with prioritizing the new website 👍

Upgrading scalameta resulted in a "ClassNotFoundException" for a scalacheck class that was fixed by adding a dependency on scalacheck in the tests.

Unskip trailing commas tests, fixes #1174

gabro changed the title ~~Add trailingCommas option and implement the "true" case~~ Add trailingCommas option May 11, 2018

Add trailingCommas option and implement the "true" case

00cce2d

gabro force-pushed the trailingCommas branch 2 times, most recently from 7fe8845 to 5efa3fd Compare May 11, 2018 15:03

Implement removal when trailingCommas = false

71d3e5f

gabro force-pushed the trailingCommas branch 2 times, most recently from 70a1978 to 20ed2fb Compare May 11, 2018 15:48

gabro added 2 commits May 11, 2018 18:04

Handle braces, brackets and check for dialect

276d34e

Fix broken test

f23ecc3

gabro force-pushed the trailingCommas branch from 20ed2fb to f23ecc3 Compare May 11, 2018 16:05

gabro commented May 11, 2018

View reviewed changes

olafurpg requested changes May 11, 2018

View reviewed changes

Introduce trailingCommas = always|never|preserve and default to preserve

07f9f92

gabro force-pushed the trailingCommas branch from 50d2c9c to 07f9f92 Compare May 11, 2018 17:01

Handle comma removal when joining into a single line and refactor

ab17688

Handle comments after trailing commas

6fc3da4

The tests are skipped because of an upstream parser bug in Scalameta. See scalameta/scalameta#1531

gabro commented May 12, 2018

View reviewed changes

s/ClosingBracket/CloseDelim for consistency with Scalameta internals

c5130ca

gabro force-pushed the trailingCommas branch from 7837e7d to c5130ca Compare May 12, 2018 10:10

Fix trailing commas followed by comments

19618d5

gabro commented May 13, 2018

View reviewed changes

olafurpg reviewed May 13, 2018

View reviewed changes

olafurpg mentioned this pull request May 13, 2018

optIn.configStyleArguments = false breaks with trailing commas #1060

Closed

Rename trailing commas test names for consistency

8ee953c

gabro requested a review from olafurpg May 14, 2018 08:32

Restrict trailingCommas handling to whitelist of owners

e61d474

gabro force-pushed the trailingCommas branch from 98d2395 to e61d474 Compare May 14, 2018 09:13

gabro commented May 14, 2018

View reviewed changes

olafurpg requested changes May 14, 2018

View reviewed changes

olafurpg mentioned this pull request May 14, 2018

Unskip trailing comma tests after next scalameta release #1182

Closed

Rely on existing isDefnOrCallSite method and add more tests

a9f3263

gabro commented May 14, 2018

View reviewed changes

olafurpg approved these changes May 14, 2018

View reviewed changes

Format

c69a09b

gabro merged commit 2a7dc2a into scalameta:master May 14, 2018

gabro deleted the trailingCommas branch May 14, 2018 14:15

olafurpg reviewed May 14, 2018

View reviewed changes

olafurpg mentioned this pull request May 14, 2018

Enable trailingCommas = always by default? #1184

Closed

olafurpg added a commit that referenced this pull request May 27, 2018

Merge pull request #1197 from olafurpg/1182

25390bd

Unskip trailing commas tests, fixes #1174

olafurpg added this to the v1.6.0 milestone May 27, 2018

@@ @@ -7,7 +7,7 @@ foo( @@
               >>>
               foo(
 ,
-,
+

		@@ -318,6 +319,11 @@ object TreeOps {
		def isDefnOrCallSite(tree: Tree): Boolean =

Add trailingCommas option #1174

Add trailingCommas option #1174

Conversation

gabro commented May 11, 2018 • edited by olafurpg Loading

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabro commented May 11, 2018 • edited Loading

gabro commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabro commented May 12, 2018 • edited Loading

gabro commented May 13, 2018

gabro May 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabro commented May 14, 2018

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabro commented May 14, 2018

olafurpg left a comment

Choose a reason for hiding this comment

gabro commented May 14, 2018

olafurpg commented May 14, 2018

gabro commented May 11, 2018 •

edited by olafurpg

Loading

gabro commented May 11, 2018 •

edited

Loading

gabro commented May 12, 2018 •

edited

Loading

gabro May 13, 2018 •

edited

Loading