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

Fix: `comma-dangle` was confused by type annotations (fixes #7370) #7372

Merged
merged 2 commits into from Oct 17, 2016

Conversation

Projects
None yet
8 participants
@mysticatea
Copy link
Member

commented Oct 15, 2016

What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix (template)

See #7370 for the template.

What changes did you make? (Give an overview)

Since c8796e9, comma-dangle rule have been confused by type annotations.
This PR modifies the rule as supporting type annotations correctly. (it just removes optimization code.)

This follows #6723 about the way to test type annotations.
The AST was created by babel-eslint of http://astexplorer.net/ .

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

Nothing in particular.

@mention-bot

This comment has been minimized.

Copy link

commented Oct 15, 2016

@mysticatea, thanks for your PR! By analyzing the history of the files in this pull request, we identified @keithamus, @gimenete and @lo1tuma to be potential reviewers.

@eslintbot

This comment has been minimized.

Copy link

commented Oct 15, 2016

LGTM

@not-an-aardvark
Copy link
Member

left a comment

LGTM, thanks!

@btmills
Copy link
Member

left a comment

LGTM

@platinumazure

This comment has been minimized.

Copy link
Member

commented Oct 17, 2016

LGTM, only question I would have is if auto-fix is supported for this rule/this case (if so, the invalid tests should have output).

@platinumazure
Copy link
Member

left a comment

Could you please add output to tests? (Even if auto-fix doesn't work for these scenarios, it'd be good to see output with the same value as code so we know that no fixes should be occurring.)

@eslintbot

This comment has been minimized.

Copy link

commented Oct 17, 2016

LGTM

@jquerybot jquerybot added CLA: Error and removed CLA: Valid labels Oct 17, 2016

@jquerybot jquerybot added CLA: Valid and removed CLA: Error labels Oct 17, 2016

@mysticatea mysticatea merged commit 681c78a into master Oct 17, 2016

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
jQuery Foundation CLA All authors have signed the CLA
Details

@mysticatea mysticatea deleted the issue7370 branch Oct 17, 2016

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

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