Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

trailing-comma: add option to forbid trailing comma after rest #3176

Merged
merged 6 commits into from
Sep 14, 2017
Merged

trailing-comma: add option to forbid trailing comma after rest #3176

merged 6 commits into from
Sep 14, 2017

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Aug 28, 2017

PR checklist

Overview of change:

Add an option "specCompliant" to the rule. Enabling that option forbids trailing commas after object and array rest as well as rest parameters. That makes the linted code compatible with the ES spec and the implementation in JS VMs.
Fixes: #3147

Is there anything you'd like reviewers to focus on?

Do you know a better option name than specCompliant?

CHANGELOG.md entry:

[new-rule-option] trailing-comma adds option "esSpecCompliant" to make it compatible with the ES spec regarding trailing commas after object/array rest and rest parameters.

@IllusionMH
Copy link
Contributor

IllusionMH commented Aug 28, 2017

Shouldn't be the problem, but tests can be added for combination of arguments destructuring and regular params later and vice versa

function combined([a, ...arr], {b, ...obj}, regular) {}
                                                   ~ [Missing trailing comma]

function combined([a, ...arr,], {b, ...obj,}, regular,) {}
                            ~ [Forbidden trailing comma]
                                          ~ [Forbidden trailing comma]

UPD. Is it worth to consider adding specCompliant: true to tslint:recommended config at least for next major update?

@ajafff
Copy link
Contributor Author

ajafff commented Sep 12, 2017

@IllusionMH I just added tests as suggested

Is it worth to consider adding specCompliant: true to tslint:recommended config at least for next major update?

We should definitely enable it for .js files. And to keep it consistent we should also enable it for .ts files.
Users who want to use trailing commas everywhere in their typescript files, can disable it using specCompliant: false.

I'm thinking about renaming the option to something like "mode": "spec" and "mode": "consistent". Thoughts?

@adidahiya
Copy link
Contributor

What does "mode": "consistent" convey? "spec compliant" sounds a little more intuitive to me, but I would suggest making it even more verbose as esSpecCompliant

@ajafff
Copy link
Contributor Author

ajafff commented Sep 13, 2017

What does "mode": "consistent" convey?

It would enable you to consistently use traling commas everywhere in your TypeScript code. Not a great idea on second thought, since it's inconsistent with how JavaScript works.

Renamed to "esSpecCompliant" as suggested.

@adidahiya
Copy link
Contributor

master is broken (semantic merge conflict?) but this PR looks gtg

@adidahiya adidahiya merged commit 404c06a into palantir:master Sep 14, 2017
@ajafff
Copy link
Contributor Author

ajafff commented Sep 14, 2017

That's not a merge conflict. The typescript@next changed the error message (and detection algorithm) for unused variables.
I'm working on a fix right now.

@ajafff ajafff deleted the trailing-comma-rest branch September 14, 2017 18:06
@thgreasi
Copy link

Woohoo!

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

Successfully merging this pull request may close these issues.

trailingComma is required after rest parameter
4 participants