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

Rule Change Consideration: Allow trailing commas #586

Closed
joshburgess opened this issue Aug 11, 2016 · 11 comments

Comments

@joshburgess
Copy link

commented Aug 11, 2016

Hi, since I started using Standard, this is one of the only rules I overwrite. Many people like consistently using trailing commas in arrays and objects due to it making git diffs simpler/more clear, and this is valid in all current browsers as far as I know. Now, even trailing commas in function params & calls are going to be valid.

See "Trailing commas in function parameter lists and calls" here:

https://github.com/tc39/proposals/blob/master/finished-proposals.md

This is the one inconsistency about using them that I didn't like... function param lists that span multiple lines... but soon that will no longer be a problem.

Do you think this changes anything in terms of the linting rule? I'm not sure why they were originally disallowed. Was it due to problems with older browsers?

@dougwilson

This comment has been minimized.

Copy link

commented Aug 11, 2016

Previous discussion for reference: #240

@feross

This comment has been minimized.

Copy link
Member

commented Aug 12, 2016

@joshburgess Thanks for the thoughtful issue.

If there's two ways to do something and there's no difference in correctness or chance of introducing a bug, then standard tends to 'just pick something'. So, relaxing the rule to allow either style isn't really a viable option.

Given that it's gotta be either 'disallow' or 'enforce', I think disallow is the least controversial option. Enforcing trailing commas would confuse a lot of beginners to programming or JS, IMO.

@feross feross closed this Aug 12, 2016

@feross feross added the question label Aug 12, 2016

@zwhitchcox

This comment has been minimized.

Copy link

commented Sep 14, 2016

This does matter...it confuses the git diffs whenever you change something...it's a red herring for something that didn't change...

e.g., if you changed added another property to this object

{
  prop1: 1,
  prop2: 2
}

and I followed standard, the git diff would look like this:

{
  prop1: 1,
-  prop2: 2
+  prop2: 2,
+  prop3:3
}

whereas, if you have a trailing comma, it would look like this:

{
  prop1: 1,
  prop2: 2
+  prop3:3,
}

I realize that's not a huge deal, but it is undeniably better as it makes your git logs more accurate. In fact, in es2017, they changed the spec to allow for trailing commas in functions to allow for this. And this is single-handedly the reason I cannot use this standard.

It's confusing for experience programmers.

@dcousens dcousens added the i disagree label Sep 14, 2016

@dcousens

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

n = number of standard users 
h = number of hours lost to fix standard errors if changed
k = hours "gained" by `git diff` reduction

n*h > k * 1000

I just don't think its worth it. But maybe --fixup can help here...

@zwhitchcox

This comment has been minimized.

Copy link

commented Sep 14, 2016

This particular problem could be solved by --fix...so, maybe if there's an easy fix for it, then it's all right..and just put in a console warning whenever they upgrade or something

@zwhitchcox

This comment has been minimized.

Copy link

commented Sep 14, 2016

Also, I couldn't use it because of requiring parentheses after a new constructor...and maybe there would be more, so, maybe a standard linter is not for me I guess

@LinusU

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

Also, I couldn't use it because of requiring parentheses after a new constructor

why is that a problem? they are syntacticly the same, right?

@zwhitchcox

This comment has been minimized.

Copy link

commented Sep 14, 2016

Yeah, but I view them the same way as semicolons--unecessary...I guess I could give that one up though....maybe even the trailing comma too idk

@feross

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

It confuses the git diffs whenever you change something...it's a red herring for something that didn't change...

This is a lost cause. Git makes huge mistakes in rendering diffs all the time, requiring a careful line-by-line examination. Good diffing is really hard. This example of how trailing commas enable "more accurate diffs" just isn't very compelling in light of the way most diffs end up looking.

@yantakus

This comment has been minimized.

Copy link

commented Nov 2, 2016

Enforcing trailing commas would confuse a lot of beginners to programming or JS, IMO.

@feross You are absolutely inconsistent. You chose to omit semicolons. This is 1000 times more confusing (not even for beginners), than trailing commas. It may cause errors in code, while trailing commas can not. And they have a great benefit of making git diffs much more clear and neat.

I'm very excited with the project, but some decisions seem really strange and inconsistent for me.

You could have a checklist:

  • Is the rule confusing?
  • Does it bring any value?
  • Does it introduce any inconveniences while typing / copy-pasting code?
  • Does it play nice together with other rules (to make the rules in general more consistent)?
  • etc...

And go through it for both enforcing and disallowing a rule.

And then you'll be able to answer: "This rule has more advantages according to our check-list" instead of the silly "Just pick something".

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2016

@web2style I feel you're being borderline rude, despite what are probably good intentions

Locking due to bikeshed; I feel it's been clear that this is not gonna happen. If anyone has different opinions, feel free to fork, roll your own and merrily maintain that

@standard standard locked and limited conversation to collaborators Nov 2, 2016

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