SIP-27: Trailing Commas, revised document #625

Merged
merged 11 commits into from Nov 25, 2016

Projects

None yet

4 participants

@dwijnand
Member

No description provided.

dwijnand added some commits Nov 12, 2016
@dwijnand dwijnand SIP-27: Simplify the History 16bfa8d
@dwijnand dwijnand SIP-27: Move back to pending/under review d51d757
@dwijnand dwijnand SIP-27: Major rework: multi-line, 2 variants & spec 049df35
@dwijnand dwijnand SIP-27: Re-word my recommendation for the first variant 3d9f0ea
@dwijnand dwijnand SIP-27: Be more explicit about multi-line 0066ac4
@dwijnand
Member

The move back to pending is postponed to ease review as GitHub didn't recognize it as a file rename.

@dwijnand
Member

Review by @xeno-by if possible, please

dwijnand added some commits Nov 21, 2016
@dwijnand dwijnand SIP-27: Re-word that the impl PR == the current proposal 206fba0
@dwijnand dwijnand SIP-27: Add links to PR numbers
.. for those who don't want to re-read the whole document but instead want to see the diff of what changed.
15e15e3
@dwijnand
Member

The next SIP meeting is next week Tuesday (29th).

If this doesn't get any review, I'm keen to get it merged some time before the meeting to give the committee members a nicely rendered, updated version of the document.

If it were up to me I'd say give it another day and merge it tomorrow if nothing changes, leaving a week for pre-meeting review/study.

Thanks.

@jvican
Member
jvican commented Nov 21, 2016

I agree with the suggestion. Will merge tomorrow if people in the Community don't have anything else to say 😄.

vote-text: The following proposal needs to be updated, since only the specialized case version (with new lines) has been accepted. For more information, check the <a href="http://docs.scala-lang.org/sips/minutes/sip-20th-september-minutes.html">minutes</a>.
---
+// TODO: Move from sips/completed to sips/pending
+
@xeno-by
xeno-by Nov 22, 2016 Member

Do we want to leave the TODO here?

@dwijnand
dwijnand Nov 24, 2016 Member

I'm removing the TODO and moving the document in #631, which can be merged after this is merged.

bar,
-)
+) // error: illegal start of simple expression
{% endhighlight %}
### Reduce diff noise
@xeno-by
xeno-by Nov 22, 2016 Member

Should this be "Diff noise reduction" to be consistent with other section titles?

-There are a number of different elements of the Scala syntax that are comma separated, but instead of changing them all a subset of the more useful ones was chosen:
+### Multi-line
+
+It is not the intent of introducing trailing commas to promote a code style such as:
@xeno-by
xeno-by Nov 22, 2016 Member

Could you briefly explain why?

-From the spec these are:
+2. The second variant adds trailing comma support to the whole grammar (again, only for multi-line), which means more consistency, but also supporting trailing commas in places that doesn't really need it, such as `ids`.
@xeno-by
xeno-by Nov 22, 2016 Member

What do you mean by "doesn't really need it"?

## Implementation
-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][].
+The implementation of trailing commas is a matter of changing some of the implementation of Scala's parser. An implementation of this proposal can be found at [scala/scala#5245][].
@xeno-by
xeno-by Nov 22, 2016 Member

How about "is limited to changing Parsers.scala in the Scala compiler.`?

@dwijnand dwijnand SIP-27: Consistent section titles 3f11780
@jvican
Member
jvican commented Nov 24, 2016

@dwijnand Let me know when you're done with all the changes, and I will merge it.

dwijnand added some commits Nov 24, 2016
@dwijnand dwijnand SIP-27: Briefly explain why no single-line trailing commas d1ebb53
@dwijnand dwijnand SIP-27: Try & clarify the drawbacks of whole grammar trailing comma s…
…upport
fbf4172
@dwijnand dwijnand SIP-27: Clarify the description of the proposed implementation afd7091
@dwijnand
Member

Thank you @xeno-by for the review, see my pushed changes.

@jvican Let's merge either tomorrow or when @xeno-by approves, which ever comes first. Thanks.

@xeno-by
Member
xeno-by commented Nov 25, 2016

LGTM

@jvican jvican merged commit 4181d9f into scala:master Nov 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dwijnand dwijnand deleted the unknown repository branch Nov 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment