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

tslint: Enable trailing-comma #2407

Closed
wants to merge 1 commit into from
Closed

tslint: Enable trailing-comma #2407

wants to merge 1 commit into from

Conversation

simark
Copy link
Contributor

@simark simark commented Jul 19, 2018

I like when people use a trailing comma in comma-separated definitions.
Turns out there is a tslint rule for it! And it also has an autofixer,
so I ran tslint --fix to generate this patch. I sampled a few files and
the results seems good.

For example, I would prefer this:

const foo = [
  A,
  B,
  C,
];

rather than:

const foo = [
  A,
  B,
  C
];

The main reason is that it simplifies diffs where you add a new item.
You don't need to change the line containing "C".

With the "multiline" setting, things like this are not affected:

const foo = [ A, B, C ];

If you want to add "D", you'll need to change the line anyway, so the
comma would not help here.

Signed-off-by: Simon Marchi simon.marchi@ericsson.com

I like when people use a trailing comma in comma-separated definitions.
Turns out there is a tslint rule for it!  And it also has an autofixer,
so I ran tslint --fix to generate this patch.  I sampled a few files and
the results seems good.

For example, I would prefer this:

const foo = [
  A,
  B,
  C,
];

rather than:

const foo = [
  A,
  B,
  C
];

The main reason is that it simplifies diffs where you add a new item.
You don't need to change the line containing "C".

With the "multiline" setting, things like this are not affected:

const foo = [ A, B, C ];

If you want to add "D", you'll need to change the line anyway, so the
comma would not help here.

Signed-off-by: Simon Marchi <simon.marchi@ericsson.com>
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

This is more consistent, looks like a quick change, and the tests still pass: LGTM.

@akosyakov
Copy link
Member

How about asking the whole team first? changes in code styles should not be a preference of one person

@simark
Copy link
Contributor Author

simark commented Jul 20, 2018

How about asking the whole team first? changes in code styles should not be a preference of one person

Right, this is the place to discuss it.

@kittaakos
Copy link
Contributor

Right, this is the place to discuss it.

I do not like it because it feels like you forgot something. But if the majority wants it, let's do it.

@akosyakov
Copy link
Member

Right, this is the place to discuss it.

I doubt that everybody is watching all PRs. Please discuss it on the dev meeting when everybody is back from vacations.

I do not like it because it feels like you forgot something. But if the majority wants it, let's do it.

I agree with @kittaakos that it does not look nice and IMHO real value is minimal.

@MarkZ3
Copy link
Contributor

MarkZ3 commented Jul 20, 2018

One of the reason I like it is that it can help with merge conflicts. If two different commits add a last item then you don't need to fix the comma.
It also makes it a bit easier to add (copy/pasting) and to reorder things.
In the end, I don't think it's a big difference so I'm okay with either.
I agree that it makes sense to discuss this in the meeting as this can be controversial.

@simark simark closed this Jul 26, 2018
@marcdumais-work marcdumais-work deleted the comma branch February 6, 2020 16:07
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 this pull request may close these issues.

5 participants