New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trailing Comma Support #262

Merged
merged 3 commits into from Feb 8, 2019

Conversation

Projects
None yet
9 participants
@marekzebrowski
Copy link
Contributor

marekzebrowski commented Nov 11, 2017

#260

@godenji

This comment has been minimized.

Copy link
Collaborator

godenji commented Nov 11, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

marekzebrowski commented Jan 4, 2018

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

This comment has been minimized.

Copy link
Collaborator

godenji commented Jan 4, 2018

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

This comment has been minimized.

Copy link

c-dante commented Feb 5, 2018

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

This comment has been minimized.

Copy link

Andrei-Pozolotin commented Feb 21, 2018

+1

@quicquid

This comment has been minimized.

Copy link

quicquid commented Aug 22, 2018

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 marekzebrowski force-pushed the marekzebrowski:trailing-coma-support branch from fbae075 to d270248 Aug 27, 2018

@marekzebrowski

This comment has been minimized.

Copy link
Contributor Author

marekzebrowski commented Aug 27, 2018

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

This comment has been minimized.

Copy link

tOverney commented Sep 19, 2018

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

@JoshRosen

This comment has been minimized.

Copy link

JoshRosen commented Oct 6, 2018

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 scalastyle/scalastyle#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

This comment has been minimized.

Copy link
Contributor

SethTisue commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Collaborator

godenji commented Feb 6, 2019

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

@jrudolph

This comment has been minimized.

Copy link
Contributor

jrudolph commented Feb 7, 2019

Yay, let's get out of the coma!

@godenji godenji changed the title Trailing coma support Trailing Comma Support Feb 8, 2019

@godenji godenji merged commit 78c655f into scala-ide:master Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment