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

Respect "Commas first" rule #464

Closed
garex opened this issue Mar 20, 2016 · 9 comments

Comments

@garex
Copy link

commented Mar 20, 2016

@see https://docs.npmjs.com/misc/coding-style#comma-first

var magicWords = [ 'abracadabra'
                 , 'gesundheit'
                 , 'ventrilo'
                 ]
  , spells = { 'fireball' : function () { setOnFire() }
             , 'water' : function () { putOut() }
             }
  , a = 1
  , b = 'abc'
  , etc
  , somethingElse
@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2016

Thanks for opening this issue. The reply here is similar as on #400

Sorry, we can't even consider this change because it would break 100% of the packages that use standard today.

If you're using standard, you can't even make this error, because it's invalid JavaScript, so you'll get a parse error when you run the tool, and probably the first time you try to run your app.

I would be against loosening this rule too, as I find comma-first particularly hard to read. Perhaps others would disagree, but in this case we've picked something and gone with it. If this is a deal breaker, then perhaps standard is not for you. I hope you understand. Thanks!

@garex

This comment has been minimized.

Copy link
Author

commented Mar 20, 2016

I also think that this is not too good to read. But. There is one but:

More clear diffs.

When default comma-last rule used, we have this in diffs:

otherItem,
- existingItem
+ existingItem,
+ newItem

But in comma-first case this will be more clear and just one line changes:

, otherItem
, existingItem
+ newItem

And we clearly see, that newItem was added. If you are respecting diff-readers, you should remeber this case.

@LinusU

This comment has been minimized.

Copy link
Member

commented Mar 20, 2016

@garex You have only moved the problem to the first line instead of the last though. Another option would be to allow trailing comma, it doesn't look nearly as bad and handle diffs very nice as well...

@garex

This comment has been minimized.

Copy link
Author

commented Mar 20, 2016

@LinusU, trailing comma is a syntax error for objects and arrays. See #400

@LinusU

This comment has been minimized.

Copy link
Member

commented Mar 20, 2016

No it isn't. I think that what @feross means is that the error of missing a comma will be caught by standard.

The only place it isn't allowed is in arguments to a function, but there is a proposal up to start allowing that...

@garex

This comment has been minimized.

Copy link
Author

commented Mar 20, 2016

Now it isn't.

@LinusU Are you sure? In which popular browsers this is not error?

@LinusU

This comment has been minimized.

Copy link
Member

commented Mar 21, 2016

var arr = [
  1,
  2,
]

var obj = {
  a: 1,
  b: 2,
}

This works in Node.js, Safari and Chrome; and probably all other browsers/runtimes as well.

@garex

This comment has been minimized.

Copy link
Author

commented Mar 21, 2016

@LinusU see these nice answers: http://stackoverflow.com/questions/7246618/trailing-commas-in-javascript

TL;DR

If you are dont' care about compatibility before js5-browsers, then trailing commas are ok for you.

@matthew-dean

This comment has been minimized.

Copy link

commented Oct 7, 2016

Comma-first looks odd at first to the human brain when you're used to something else, but pretty quickly it's advantages become obvious when using it. (Or did to me.) Mostly, I stopped making as many missing-comma errors. Pretty much not at all, since missing commas are impossible not to spot. See: https://gist.github.com/isaacs/357981

The logic about cleanliness in omitting semi-colons at the end of statements applies here too. Namely, it feels weird at first, but the code appears cleaner and is easier to use. I wouldn't just argue it should be allowed. I would say this should be the standard. The advantages are many, and the only disadvantage for the individual developer is "it's not what i'm used to". But there may be many items in this standard that an individual dev is not used to.

Try comma-first for a while! You won't go back.

As to this:

If you're using standard, you can't even make this error, because it's invalid JavaScript

Not sure why this was quoted, since that's true for #400 but completely false here.

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants
You can’t perform that action at this time.