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 notice behavior for trailing-comma esSpecCompliant #323

Merged
merged 2 commits into from
Feb 15, 2020
Merged

Change notice behavior for trailing-comma esSpecCompliant #323

merged 2 commits into from
Feb 15, 2020

Conversation

MatthiasKunnen
Copy link
Contributor

@MatthiasKunnen MatthiasKunnen commented Feb 7, 2020

PR Checklist

Overview

I've changed the notice behavior of esSpecCompliant property for rule trailing-comma. Previously, a notice would be given when the esSpecCompliant was present. Since eslint only supports esSpecCompliant == true no notice should be given in that instance.

After this change, a notice will be given when esSpecCompliant is either undefined or false.

eslint only supports esSpecCompliant true.
@JoshuaKGoldberg
Copy link
Member

👋 @MatthiasKunnen, thanks for reporting the issue and opening a PR!

I'm a little confused. TSLint's trailing-comma docs states:

To align this rule with the ECMAScript specification that is implemented in modern JavaScript VMs, there is a third option esSpecCompliant. Set this option to true to disallow trailing comma on object and array rest and rest parameters.

...but ESLint's comma-dangle docs look like they prefer allowing the trailing comma.

Per the PR template, would you mind filing an issue to discuss the changes? In general it'd be preferable to discuss whether changes should happen in issues, and save PRs for just the implementation changes.
(I can go ahead and do that if you don't have time to soon)

@JoshuaKGoldberg JoshuaKGoldberg added the status: needs investigation Let's dig deeper into this before drawing conclusions. label Feb 10, 2020
Only warn when esSpecCompliant is specifically set to false and assume
true since it's the desired behavior.
@MatthiasKunnen
Copy link
Contributor Author

You're absolutely correct about both implementations.

Both TSLint and ESLint implemented their respective rules in a way that did not deal with rest params. Both changed their rules to deal with rest params but they went separate directions. TSLint decided that old versions of typescript should not be broken as they allowed the trailing comma so they added an option. ESLint, however, decided since a trailing comma after a rest param is a syntax error, a breaking change was okay and they since disallow a comma after a rest param (discussion here eslint/eslint#7297).

TSLint actually had a discussion about removing the esSpecCompliant option and disallowing the trailing comma after a rest param which would result in the same behavior as ESLint (see palantir/tslint#4172). That issue got closed before any action could be taken due to TSLint's deprecation. The issue mentions the following:

esSpecCompliant: true is enabled in all the built-in configs, tslint:all, tslint:recommended, and tslint:latest, which I believe should be sufficient for most users. we can change the behavior of the rule to make esSpecCompliant: true the default behavior in v6.0

We can thus assume that esSpecCompliant will be true in nearly all configs that have the trailing-comma rule. Right now, these configs will receive warnings about ESLint's lack of such a feature. This, however, won't be a problem and for that reason this PR changes the conversion in order to default esSpecCompliant to true and warn when it's specifically set to false.

I hope that this explanation is clear as to the purpose of the PR and the underlying reasons. If not I can make an issue.

@JoshuaKGoldberg
Copy link
Member

If not I can make an issue.

Yes please!

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Excellent. Thanks so much for persevering through my lack of understanding! 💪

@JoshuaKGoldberg JoshuaKGoldberg merged commit a0fbef4 into typescript-eslint:master Feb 15, 2020
@MatthiasKunnen MatthiasKunnen deleted the trailing-comma-es-spec-compliance branch February 19, 2020 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs investigation Let's dig deeper into this before drawing conclusions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing notifice behavior for trailing-comma, esSpecCompliant option
2 participants