-
Notifications
You must be signed in to change notification settings - Fork 1k
SIP-27 Trailing Commas #533
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
Conversation
|
||
The implementation is a simple change to the parser, allowing for a trailing comma, for the groups detailed above, and has been proposed in [scala/scala#5245][]. | ||
|
||
## Drawbacks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth to mention that tooling/parsers will have to catch up. I can think of intellij-scala, scalariform, scala.meta and scalaparse.
(I would love to have trailing commas so big +1 from me.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By scalaparse do you mean https://github.com/lihaoyi/fastparse/tree/master/scalaparse?
Btw it was my intent to submit changes to those as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I meant fastparse/scalaparse. My only comment was to include this as a drawback, since updating those tools will require some work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olafurpg I didn't add it because I thought it might be true for all SIPs, but then maybe not. Added, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should mention that it massively simplifies generating scala source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Hello @dwijnand! Welcome to the new edition of the SIP process. My name's Eugene, and I'm going to be your reviewer. This means that I will be the contact person between you and the SIP committee, outlining your progress and providing you with our feedback. During the SIP meeting on 13 Jul 2016, I presented your proposal to the committee according to the "Formal presentation" phase described in the new rules (http://docs.scala-lang.org/sips/sip-submission.html). At this phase, we are supposed to decide whether following through your SIP is a good idea in principle. I have good news. We decided that your proposal has the potential to move Scala in the right direction, so we'd like to clear the "Formal presentation" phase and move the proposal to "Formal evaluation". We will now merge this pull request as SIP-27 and will proceed with evaluation. During the "Formal evaluation" phase, we will work with you to make sure that the proposal reaches its maximum potential. When we're sure that we have a comprehensive understanding of your proposal and its consequences on Scala, we will have a final vote and will announce our verdict. As the first step of the evaluation process, we would like to ask you to address the following feedback:
Thank you for contributing to the future of Scala. Looking forward to hearing from you again! |
Hey @xeno-by, that's excellent news! I'll spend more time on this soon, but I have a quick question: should I really jump to updating my implementation for all cases? Because I think there are some parts of the grammar where trailing commas don't really make sense nor are common enough to be worth doing it anyways, such as |
Thank you for your swift reply! Please also ask further questions in this thread - I believe that open discussion greatly benefits language evolution. What I meant is that I think it's a good idea to explore all cases, both theoretically and practically. As you rightfully pointed out, there may be cases where trailing commas aren't worth it. Please feel free to ignore these cases in your implementation. But in any case do document your findings - I'm confident that they will be of help to our subsequent discussions. |
Hello @dwijnand, the design and implementation of this SIP will be discussed in the next SIP meeting on 10th August. I encourage you to provide more insights into the possible corner cases of your implementation and how you would handle them. It would be good to update this document to reflect those and explain all the interactions with existing language features. This will surely speed up the proceess and improve the discussions in our meeting, making them more productive. For more information, please get in touch with your reviewer @xeno-by. |
I didn't realise it was so soon, but, yes, I'll make sure to respond to the initial round of feedback, such that this SIP can (hopefully) progress next meeting. Wasn't this PR meant to be merged if it's provisionally accepted as is, and then go through some finessing and final decision? |
Yes Dale, that's the plan, I'm planning to merge it soon. Please, note that even if this PR is merged, all the feedback will still go into this PR, so that all the discussion is centralized in the same place. |
I've studied the relevant parts of the Scala grammar with regards to adding trailing commas (breakdown and my thoughts below). Summary (TL;DR)In my opinion not all comma-separated elements of the grammar make sense to support trailing commas, but they could, for across-the-board consistency. I leave it to the discretion of the SIP committee to let me know what they think. I focussed particularly on the interaction between trailing commas and My proposal for these ambiguous cases is to follow @sjrd's advice and make them not compile, like they don't currently. The alternative is make it syntax for Once the SIP committee responds to these notes I'll incorporate them into the SIP document. Breakdown
|
Thanks for the thorough explanation Dale. As a next step, it would be good to add this to the SIP document. We'll discuss this in the next meeting. |
) | ||
{% endhighlight %} | ||
|
||
Also such feature would massively simplify generating Scala source code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/massively/modestly/ IMO — "massively" just seems like hyperbole. or is it really that big an issue in your experience? (I do have some experience writing code generators that omitted trailing commas and found it annoying, but really not that a big deal, either.)
@SethTisue thank you very much for the review, I've made some changes from your feedback. I hope you think that's more appropriate and let me know if not. @jvican Thank you, I hope you're ok with me updating the document with the breakdown once the SIP committee has provided some feedback. |
I am not very enthusiastic about this proposal for a number of reasons.
I've used this feature in Julia and I don't like it; when it's very important to know the size of one's vector, not having an at-a-glance count of the number of commas be one less than the length is a minor but unwelcome distraction. I can't see why I'd like it any better in Scala. That said, if it is decided that something like this is desired, I would much prefer if commas and semicolons acted similarly. That is, when you can have multiple semicolons you can throw as many of them in as you want between expressions as long as you have at least one (and empty expressions don't count). For instance: scala> val x = if (true) {;;; 7 ;;;} else 2
x: Int = 7 Actually using this capability is rare, but it is regular in that semicolon acts like a type of super-whitespace; just as Scala doesn't care about how much whitespace there is or whether there's any at all, as long as the tokens are delimited somehow, it also doesn't care about just how many semicolons there are or whether there's any statement at all. Commas now are special in that they always and only separate items of some sort. Making them obey the same conceptual rules as semicolons and whitespace would, I think, simplify the language conceptually overall. There's only one separator concept; you just have three kinds of separation (token, item, expression, ) and thus need different tokens to distinguish them (with newline usually sufficing for semicolon). Thus, we would allow val ,,,,, x= if (true) Seq(,,, 7 ,,,) else Seq(2) Not that you should write it this way, but I think picking the "nice" syntax from the spectrum of logically consistent syntax is the job more of linters and style checkers than the language specification itself. (Unless the language is Python.) Again, I think the benefits of having comma be a singular separator outweigh it being conceptually consistent with whitespace and semicolon, but I think the existing proposal is less desirable than either. |
If this proposal were to go ahead it would at least be good to have a compiler warning for multiple trailing commas/semi-colons eg |
I doubt commas could become optional like semi-colons, as that would make it ambiguous when an expressions spans multiple lines or not. So we can't make the rules identical to I don't see why the grammar would be much more complicated. It would just need another case terminating repetition including a comma. I think one of the main reasons for the proposal of this change is what some perceive as a usability problem. Writing text in an editor is the user interface for developing programs. Developers may use this interface (text) differently, but there seems to be a significant group (I among them), who have the following problem: When parameters or arguments are indented one per line, commenting out the last one requires also removing the trailing comma from the previous line. We perceive this as a usability problem, an irregularity, a distraction from getting stuff done. This might not be the biggest problem there is, but annoying enough to a good number of people to bother making an effort to change it. |
I agree with Rex's sentiments here. We've come to regret having more than one way to do things syntactically (e.g. procedure syntax). IntelliJ users can move elements within a comma separated list with the "Code -> Move element {left, right}" (or the corresponding shortcut): I'm also uncomfortable about the following code that reports a syntax error into something that typechecks.
Incidentally, I found a similar enhancement request for Java that suggested to generalize its support trailing commas in array initializers to all comma separated parts of the syntax. That request wasn't accepted, although there wasn't much constructive discussion of the reasons why. Scala has a history on mimicing Java's syntactic decisions closely. |
Many of us do not use IntelliJ for usability reasons in particular responsiveness. Different people choose different trade-offs here. Editing the last element of argument lists happens all the time. I can see your described use case, but it seems rare to me personally. But I understand your case, potential correctness problems have gravity. Everything's a trade-off. I personally tend towards usability in this particular case. Scala tries to have syntax that is easier to work with than Java, yet familiar to Java-style language folks. I see trailing commas supporting the first and yet not violating the second. |
But good point. It's not only commenting out parameters/arguments, also moving them that has this usability issue. |
@cvogt - Can't you just List(
fiddesticks /*,
humbug*/
) instead of changing the language? Alternatively, can't you fix your editor to make it easy for you so it works for all languages, not just Scala? There are fewer heavily-used editors than languages. Healing line-by-line commas is simple: if you comment out a line, and the next line is a close of some sort, and the previous line ends in a comma, comment out the comma as well as the line. If you uncomment it, and the next line is a close, and the previous line has a comment that starts with a comma, remove the comment around the comma also. Compared to semantics-aware refactoring, this is a pretty easy operation. Adding a new color for "syntactically necessary comment", and making that be close to the background color, would remove the visual distraction also. Moving lines around isn't so hard either. Moving stuff around on the same line is hard because there is nothing to cue off of except a parse of the language at that point. That's much heavier lifting for an editor / IDE. But this seems pretty esoteric for me, and as I mentioned in chat, it doesn't solve the problem of how to comment stuff out on the same line. (For that to be easier you need commas-anywhere.) |
@Ichoran this is the sort of gymnastics i've been doing for years, and it's tiresome.. |
@Ichoran This proposal has a significant number of supporters because we find exactly that a usability hinderance. People are different :). Regarding editor plugins: Many of us prefer syntax that is easily editable as much as possible without external tooling support. For everything that requires tool support we limit ourselves to tools that support this feature and have to remember how to use it and there is a limited memory budget for things like that, I'd prefer to spend on other things. Suggestion: Recognizing that a significant number of people would feel their life becomes easier with this, let's focus on how other people feel their life would become harder with this and then weigh off the trade-off. |
I disagree strongly that it's pointless, and I invite you to look at the positive feedback the original Scala PR and this SIP has attracted in order to acknowledge that a lot of people also disagree with you. I also personally think it's unnecessary hyperbole to say that this "will undo all the progress we have made in the last half decade". I care about your point of view and your feedback, but you make it difficult for me to take it seriously when you disregard obvious evidence and exagerate like that. Continuing to mock the effort put behind this SIP and importance some people give to this SIP by calling it "SIP-commas" doesn't help you either.
It's a digression, but know that aligning rows actually doesn't get nice diffs in git, but it does improve the legibility of code, in my opinion. Instead of pushing the burden of managing how to add, remove or move elements in a comma-separated list to each and every editing tool out there, I'm proposing that scalac learns how to manage trailing commas instead.
I believe that the trade-off between how much people suffer to read and ignore a trailing comma and how much people suffer to micro-managing the lack of a trailing comma when editing code is heavily in favour of the latter. It's the duty of the committee to decide and vote if they agree with me on this.
If you're asking my personal opinion, I would say it should say that trailing commas on multi-lines are accepted, for the conveniences specified in this SIP. |
I agree with you, which is why I'm proposing we restrict this change to only multi-line comma-separated elements. |
As do I. |
I am aware, but I reject it as a workaround for 3 reasons. First, as you mention, it moves the problem to the first element, which only maybe might not be a problem. Secondly, in the case where a change must happen near the first element (either by adding one or more elements before the first element, or by remove the first element, either entirely or by commenting out) it makes the change a lot harder, because it requires micro-managing the leading comma, replacing it in situ with a space. Thirdly, it's a large departure from the "normal" style - in contrast I'm only proposing an optional trailing comma. One final note, which isn't an objective reason but my own personal taste, I can't stand that style. You might be interesting to know that @olafurpg has added an experimental |
I'm personally not interesting in supporting that, but if you feel strongly enough about it, I invite you to consider creating a SIP for it. |
That's a good point, thank you, I'll add it to the SIP.
While I agree that it's a con that must be managed and considered, it's also not unique to this SIP. |
Please see my grammar breakdown, where I address the concern about tuples/HLists and Tuple1. |
No, the proposal doesn't mandate trailing commas. |
1. Questions for the SIP committee:
This would mean that this would be allowed:
but this would not:
Then there is the question of how much of the Scala grammar should support trailing commas. In, what I consider to be, decreasing order of importance: Important to support:
Less important to support:
Not important to support, except for consistency reasons:
2. Compare to the HList discussion in DottyTo my limited understanding of https://github.com/lampepfl/dotty/issues/964, I don't see any conflict between my proposal and Dotty's proposal. 3. Salvaging non-controversial partsMy initial proposal was restricted to a small subset of (what I considered) non-controversial parts of the grammar, then the SIP committee requested to explore all the cases of the grammar that have comma-separated lists, which I did. Personally I'm happy to go either way on this. 4. Two ways of doing the same thingFrom very early in this SIP it has been acknowledged that this is a drawback/trade-off. Perhaps restricting trailing commas to multi-line reduces the gravity of this drawback. |
@jvican While the details of this SIP are still being worked out, specifically given the number of questions I have for the committee, I would like to hold off from committing even more time changing (and re-changing) the specifics in the actual SIP document until the situation is clearer. You have my word though that even in the event that the SIP is rejected I would update the SIP document accordingly. |
Personally, I have far fewer issues with trailing commas only when newline is allowed. There is still the concern about whether there is a missing item, but unlike the inline case where a missing entry is likely and the editing help you get is minimal, with separate lines, a missing entry is unlikely and the editing help you get is maximal. Definitely shifts the risk/reward ratio. I remain concerned about making the specification more complex, but if it's isolated to multiple lines and phrased like semicolon inference but backwards, it feels a lot tidier. And I would make it apply always, no exceptions. If you can have something separated by commas and it can go on separate lines, you can always have a trailing comma and it's cool. |
I personally could live with that, but I'm not sure if everyone would. Also it would have to be a gradual change though, particularly given there are libraries that cross-build to multiple versions of Scala. |
I can't comfortably live with anything other than a completely consistent rule. I have too many other things to pay attention to. |
@dwijnand I'm glad to see that you have so many interesting questions for the Committee! I'll ask them in our next meeting, that coincidentally takes place at Scala World, on the 13th of September (that's next week). Thanks for your commitment, Dale. |
|
||
Given that this is a change in syntax, another drawback is that it requires changing the existing tools, such as those that parse Scala: intellij-scala, scalariform, scala.meta and scalaparse. | ||
|
||
Another drawback is that for projects that cross build to previous versions of Scala they would have to take into account that this feature wouldn't be available for those versions (assuming this feature isn't backported). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this any worse than for any other change to Scala? Even simple additions to the standard library are subject to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it's fair to mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this is noise. I was trying to figure out what made this SIP special about this. IMO this paragraph should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It leads to source incompatibility at the language level. It implies a major version bump, not only a minor one (as far as I understand Scala policy regarding that, for ex. https://twitter.com/viktorklang/status/735772498780819457)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not forward source compatible (most things aren't), but it is backwards source compatible. So an existing codebase will always compile with a new compiler. This only requires a minor version bump.
FTR, adding a method to the standard library is less compatible than that. It can always break backward source compatibility of some codebases, if they pimped a method of the same name via implicits: the new method would suddenly take over instead of the implicit pimp. If the semantics are different, this breaks code.
This change is much more compatible than adding a method, as it can never break existing codebases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjrd I specifically mentioned Cross Build Hinderance (#533 (comment)) as a drawback.
It's a drawback worth considering for this SIP in particular because the added benefits are not as feature-based, and closer to the pro/con 50:50 point than, let's say SIP-14 Futures and Promises. Sure adding Future to the standard library would prevent cross building, but it does more things.
Another reason to think about this specifically for this SIP is that the surface area that might affect this SIP is potentially all Scala sources, which I don't think can be said of just adding a method to the standard library. Closer analogy might be SIP-18 Modularizing Language Features which might have added import language.implicitConversions
etc many existing source. The potential surface area becomes relevant especially given the rise of auto formatting of Scala source using Scalariform or Scalafmt.
The anywhere-in-grammar but only-for-separate-lines variant seems very nice to me. Clearly an improvement wrt the initial proposal, IMO :) As @Ichoran put it, it definitely improves the benefit/cost ratio. |
Hello everyone! I am here to officially announce that SIP-27 Trailing Commas has been accepted by the SIP committee during the meeting on 20 Sep 2016. According to the formal rules of the process (http://docs.scala-lang.org/sips/sip-submission.html), the committee proposes that compiler maintainers include this new language feature in a future release of the Scala compiler. Moreover, I would like to ask @dwijnand to update the specification and implementation according to our discussions. Concretely, the committee has voted in favor of a proposal that allows trailing commas everywhere in the grammar given that the commas are immediately followed by a newline. @dwijnand: Please change the SIP and the prototype implementation to reflect that. This ends the SIP process for the trailing commas proposal. I would like to thank @dwijnand for the thorough work on this language proposal and everyone for the insightful discussion. It is a pleasure to collaborate on building the future of the language together. |
With regard to "ease commenting out first/last element" maybe what could solve it would be generalizing of Unit as a terminator. It would be consistent with lists ending with ::Nil Example import Here you go - add, remove, (un)comment, reorder - the explicit stub-terminator would prevent compilation error. |
@the-Arioch That doesn't generalize beyond imports. You can't do the same for actual parameters or for bindings, since an additional |
You can use sone other terminator as well, like lists using Nil or HNil |
No, The above proposal, which has been approved, uses a bare newline as this unique piece of syntax. |
scala/scala#5245 was just merged, meaning this SIP's implementation will be available as of Scala 2.12.2 (due out by the end of the Feb) |
fix a lot of broken links
This is my first SIP. I'm happy to take feedback and iterate where needed.
An initial implementation has also been proposed in scala/scala#5245.
Link to Scala grammar breakdown: #533 (comment)
Link to proposing trailing commas only for multi-line: #533 (comment)
Summary of subjective pros and cons (added by @cvogt):
Pros:
Cons:
def foo(a: Int, b: Int = 0) = 0; foo(1,)