Skip to content
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 commas 🎉 #1775

Merged
merged 5 commits into from
Jan 25, 2018
Merged

🎉 Trailing commas 🎉 #1775

merged 5 commits into from
Jan 25, 2018

Conversation

jordwalke
Copy link
Member

Intelligent printing and parsing of trailing commas on basically everything!

Please see the stack of diffs here. Each commit accomplishes some purpose and it will be much easier to understand the entire set of changes by looking at the individual commits.

@chenglou
Copy link
Member

For reference: #1549

@@ -116,13 +116,13 @@ let res =
id(
fun
| Some(x) => x + 1
| None => 0
| None => 0,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this trailing comma expected?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a trailing comma on the function argument. I’m pretty sure this is what we want. The alternative is to not print trailing commas for functions accepting only one argument.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be a lot nicer to look at with different function syntax

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if funs had braces, it would be nicer. There was discussion about adding them. In the mean time, the comma is kind of nice in that it tells you pretty clearly where to add another argument. We could also special case it so that single argument functions whose argument is a fun | do not print the trailing comma.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the mean time, the comma is kind of nice in that it tells you pretty clearly where to add another argument

This seems like a compelling argument for keeping the trailing comma.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It felt a bit odd when I saw it, but it makes sense.

I really like this change and I agree it helps with paren-fatigue 👍

@jordwalke
Copy link
Member Author

I've found that trailing commas help with a bit of paren-fatigue as well, because it helps you distinguish closing parens for arguments/tuples/variants from parens whose only purpose is to resolve precedence - at least in the case where things break onto multiple lines. It's a small help, but it is a help.

"func": a => {"a": (arg1, arg2) => arg1 + arg2}
"func": a => {
"a": (arg1, arg2) => arg1 + arg2
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems missing here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes JS objects were the only ones I didn’t get to. I’ll try to add them before landing.

@jordwalke
Copy link
Member Author

Going to merge:

Suggestions for anyone trying to extend this to other features in the future:
It's very important to make sure your printer doesn't write a check that your parser can't cash.
Meaning, if you're going to print trailing commas (or anything), make sure it can be parsed. It's tough to catch all the bugs because many times trailing commas (or other features we want to add in the future) will be omitted most of the time. To catch these cases, go into the printer and disable the part that strips out these trailing commas when lines don't break. Then run the tests. Search for idempotent failures to parse. It will catch all of them. Apply that same technique to features you add in the future that are "intelligently omitted". (Such as leading bars on switch cases etc).

@jordwalke jordwalke merged commit d0cb1a6 into master Jan 25, 2018
@jordwalke jordwalke deleted the TrailingCommas branch January 25, 2018 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants