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

Require trailing commas in multiline object and array literals? #240

Closed
lydell opened this issue Aug 29, 2015 · 20 comments
Closed

Require trailing commas in multiline object and array literals? #240

lydell opened this issue Aug 29, 2015 · 20 comments
Labels

Comments

@lydell
Copy link

lydell commented Aug 29, 2015

This is a very sensible styleguide. I don’t agree with all of it personally on a purely aesthetical level, but don’t care since I think it’s worth it because of the simplicity.

Here’s another sensible rule, that I think is a tiny bit ugly myself, but is very useful: Require trailing commas in multiline object and array literals. Why? Because it reduces diffs. When adding or removing a new item at the end of the literal you only need to modify one line instead of two.

var obj = {a: 1,} // Error: trailing comma in singleline object literal.
var obj = {a: 1} // OK
var obj = {
  a: 1 // Error: Missing trailing comma in multiline object literal.
}
var obj = {
  a: 1, // OK
}

The above examples can be extended with multiple items per literal, of course, and the same apply to array literals.

This can be implemented by setting the comma-dangle rule to [2, "always-multiline"]. As described in the comma-dangle docs, the only caveat is IE8 in certain circumstances. In my opinion you should have your build tool take care of that. Most likely you’ll minify the code, and then trailing commas will be removed anyway to reduce size. EDIT: IE8 isn't even supported by Microsoft anymore.

EDIT: As mentioned in this comment, another benefit (besides diffing and git blame-ing) is ease of editing: Want to move the last line? Be sure to add in that comma!

@dcousens
Copy link
Member

Although personally I agree with the fundamental reasons that you argue for this. As it stands, this would be so much of a breaking change, it will never be accepted.

@feross
Copy link
Member

feross commented Aug 30, 2015

Thanks for the thoughtful issue! Unfortunately, it would be a lot of work for all the existing users of standard (almost 100K downloads per month) to change their code. We can't really make breaking changes at this point without a really good reason.

Fortunately, this style choice isn't the most important since it doesn't affect code correctness or make programmers more likely to write bugs. At the end of the day you have to 'just pick something', and that's the whole philosophy of standard -- its a bunch of sensible 'just pick something' opinions.

@feross feross closed this as completed Aug 30, 2015
@lydell
Copy link
Author

lydell commented Aug 31, 2015

Right, thanks for taking your time to read and answer!

@yanivtal
Copy link

yanivtal commented Jan 8, 2016

I agree with the philosophy of this approach... and yet this choice will be responsible for millions of lines of diffs based on the number of downloads per month...

@dcousens
Copy link
Member

dcousens commented Jan 9, 2016

@yanivtal is the upgrade diff cost less than than the diff cost of leaving it the same for say, 5 years?
Probably not, but if you want to run the numbers, I'm sure it'd be an interesting read.

@gimenete
Copy link

Instead of making it mandatory at least it could stop marking it as an error. Now it says Unexpected trailing comma. (comma-dangle).

So my suggestion is: no error if you use it and no error if you don't.

What do you think guys?

@yanivtal
Copy link

i like that suggestion. but regardless, @dcousens good answer ;)

@gimenete
Copy link

As you can see here eslint/eslint#4968 I've made a PR to eslint to add a new option on comma-dangle: allow-multiline. This will warn only when using a dangle comma on single line node, but makes it optional on multiline ones.

@mattdesl
Copy link

So my suggestion is: no error if you use it and no error if you don't.

👍

Some posts on the subject.

https://medium.com/@nikgraf/why-you-should-enforce-dangling-commas-for-multiline-statements-d034c98e36f8#.yndunrwhf
http://dontkry.com/posts/code/trailing-commas.html

@feross
Copy link
Member

feross commented Apr 22, 2016

The issue with relaxing the rule is that some places in the codebase will have it, some won't. That will lead to discussion about the issue in PRs, edit wars, etc. standard should enforce one style.

@ianstormtaylor
Copy link

Every other rule seems super sensible, and tends towards consistency. And not opting to make trailing commas optional means that people have to hack around the defaults or adopt the poor decision in their codebases... seems weird given that it seems like everyone admits this was a poor decision from the start that would be changed if only it didn't cause extra work. This is the only rule I disagree with in standard, and sadly the one that will make me not adopt it.

@feross
Copy link
Member

feross commented Jun 7, 2016

If you feel that strongly about a single style rule like trailing commas, then standard is probably not for you. Consider using eslint directly with eslint-config-standard and tweak away to your hearts content if that's what you wish.

@jokeyrhyme
Copy link

Note that ESLint's --fix option will automatically migrate code in either direction regarding the comma-dangle rule. Does having a computer-assisted code migration like this change the perception of compatibility breakage?

@jokeyrhyme
Copy link

There's a thorough discussion about this subject here: https://medium.com/@nikgraf/why-you-should-enforce-dangling-commas-for-multiline-statements-d034c98e36f8

@dcousens
Copy link
Member

dcousens commented Jun 27, 2016

@jokeyrhyme

almost 100K downloads per month

and

We can't really make breaking changes at this point without a really good reason.

Are simply not compatible, --fix or not. It's just not worth it by comparison to the trivial difference in diffs it provides.
It really doesn't make that great a difference.

@lucasrcosta
Copy link

lucasrcosta commented Jul 13, 2016

2 cents here: the diff things is not as important for me as the Easier Code-Manipulations argument, which seems to be apart from this discussion. How many objects and arrays are multilined in everyone's code? Have you never had to change the order or sort some properties and spend time fixing the comma? I sure did.
So for me that's the math in discussion: developer hours! How many added hours will it take to automatically change all the missing commas from all the existing projects in a release? How many added hours we will be thrashing in next years of use of the standard fixing dangled commas?
For me, that's a really good reason.

@jokeyrhyme
Copy link

FYI: Upstream ESLint have moved "comma-dangle" from Possible Errors to Stylistic Issues, and also removed it from the "eslint:recommended" configuration:

This helps enforce the notion that this is a style thing, and Standard is a standard for code style.

@dcousens
Copy link
Member

dcousens commented Jul 13, 2016

How many objects and arrays are multilined in everyone's code? Have you never had to change the order or sort some properties and spend time fixing the comma? I sure did.

I concur, I have spent some unprecedented amount of time on this, it is especially bad when dealing with humongous JSON files that might lag out your editor.

@jokeyrhyme
Copy link

@dcousens the JSON specification does not allow trailing commas: http://www.json.org/
See the rail-road diagram for Array and Object.

JSON !== JavaScript, and I'm not sure JavaScript Standard Code Style covers JSON.

But for Objects and Arrays in JavaScript, I do not disagree with you.

@dcousens
Copy link
Member

dcousens commented Jul 13, 2016

@jokeyrhyme true indeed! Welp. I've returned to not really caring about this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Archived in project
Development

No branches or pull requests

9 participants