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

Change "comma-dangle" rule #19131

Closed
daynin opened this issue Mar 4, 2018 · 8 comments
Closed

Change "comma-dangle" rule #19131

daynin opened this issue Mar 4, 2018 · 8 comments

Comments

@daynin
Copy link
Contributor

daynin commented Mar 4, 2018

What about changing this rule in eslint config? At the moment it's only-multiline which allows but does not require trailing commas, and it can create misunderstandings in code review process.

Besides, changing this rule to always-multiline will allow us to avoid misunderstandings and make diffs cleaner

@apapirovski
Copy link
Member

There would be too much churn, I think. Also, off the top of my head I'm not even certain how far back V8 support for it goes. Would we be able to backport such a change to v6.x?

@daynin
Copy link
Contributor Author

daynin commented Mar 4, 2018

Ok, if we can't use always-multiline for some reason, let's use never instead only-multiline. I think it'll be much better

@apapirovski
Copy link
Member

There's going to be churn either way. Long-term we would likely prefer always-multiline, it just might need to be a more gradual transition.

Then again, if V6.x already supports trailing commas then if someone wanted to do that and immediately backport to all active release lines, I wouldn't personally stand in the way.

@daynin
Copy link
Contributor Author

daynin commented Mar 4, 2018

@apapirovski v6 does not support trailing commas in function declarations and calls

@daynin
Copy link
Contributor Author

daynin commented Mar 4, 2018

So, we can use this rule for objects and arrays only

@daynin
Copy link
Contributor Author

daynin commented Mar 4, 2018

@apapirovski
I can change the rule and fix all errors but

... immediately backport to all active release lines

How can I do this?

daynin added a commit to daynin/node that referenced this issue Mar 4, 2018
- change the rule
- fix all linting issues
- close nodejs#19131
@tniessen
Copy link
Member

tniessen commented Mar 5, 2018

Not sure whether there is consensus about this, I myself am certainly not a fan of dangling commas and definitely not in favor of enforcing them. There are not too many situations where this rule actually reduces the diff size and it appears unnatural to me to end a list with a separator.

@daynin
Copy link
Contributor Author

daynin commented Mar 5, 2018

I am going to close this issue due to the concerns in this PR #19133. Thank you for the discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants