search

scala-ide / scalariform

Scala source code formatter
http://scala-ide.github.com/scalariform/
MIT License
527 stars 148 forks source link

Trailing Comma Support #262

Closed marekzebrowski closed 5 years ago

marekzebrowski commented 6 years ago

https://github.com/scala-ide/scalariform/issues/260

godenji commented 6 years ago

@marekzebrowski great, thanks for the PR.

Can you please add a test to verify expected behavior?

For example, create TrailingCommasTest.scala. Do an inline and multi-line example so we know the formatting works as expected.

package scalariform.formatter

import scalariform.parser._
import scalariform.formatter.preferences._

// format: OFF
class TrailingCommasTest extends AbstractFormatterTest {

  """val foos = List(
      |  f1,
      |  f2,
      |)""" ==>
  """val foos = List(
      |  f1,
      |  f2,
      |)"""

//  format: ON

  override val debug = false

  def parse(parser: ScalaParser) = parser.nonLocalDefOrDcl()

  type Result = FullDefOrDcl

  def format(formatter: ScalaFormatter, result: Result) = formatter.format(result)(FormatterState(indentLevel = 0))

}
marekzebrowski commented 6 years ago

For sure that PR will change formatting - commas will be removed. I don't exactly have time now to work on a solution that does not change it, so you may just drop PR.

godenji commented 6 years ago

We can leave it open, clearly there are users interested in having it implemented. I too am and have been stretched thin time-wise.

Hopefully someone can step up and take the PR to the finish line.

c-dante commented 6 years ago

I made a parser spec to test this PR, and it failed on trailing comma at the parser level. I'm going to see if I can get this working with a formatting option + test.

Adding to this, ref for trailing tokens: http://docs.scala-lang.org/sips/completed/trailing-commas.html

Andrei-Pozolotin commented 6 years ago

+1

quicquid commented 6 years ago

I would be interested in keeping the comma during formatting, but even deleting it sounds better than formatting bailing out completely. Is there a chance this could be merged at some point?

marekzebrowski commented 6 years ago

I updated PR and added super basic test case. It should not blow up, it does not delete trailing commas (in most places at least). I don't have more time now to test it better at this point.

tOverney commented 5 years ago

@godenji what's the status on that one? I would really be nice to have!

JoshRosen commented 5 years ago

Is it possible to reduce scope here such that we can make incremental progress / deliver incremental value to users of Scalariform?

As @quicquid and @marekzebrowski both point out, "not crashing" when parsing trailing commas is a huge improvement by itself, even if preservation of trailing commas during reformatting doesn't work 100% perfectly. This parser crash issue is blocking support for trailing commas in Scalastyle (see https://github.com/scalastyle/scalastyle/issues/276). The current crash is also a huge usability / developer productivity issue (especially for new Scala developers) because the Expected identifier, but got Token(RPAREN,),xxx,)) error message is confusing.

As long as this current patch doesn't introduce significant correctness issues (i.e. it doesn't introduce the potential for Scalariform reformatting to change the meaning of a program) then I think it makes sense to merge it and cut a new release, thereby unblocking Scalastyle and delivering incremental value to Scalariform users.

/cc @matthewfarwell and @godenji.

SethTisue commented 5 years ago

there's clearly a lot of interest in this. curious, what's next here? is there any opposition to merging this PR? who could merge it?

this came up recently over at https://contributors.scala-lang.org/t/preliminary-developer-survey-results/2681/13

godenji commented 5 years ago

Should have time to merge this and cut a release this weekend.

jrudolph commented 5 years ago

Yay, let's get out of the coma!