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: Amend keyword-spacing to validate default keywords #11097

Merged
merged 2 commits into from Dec 3, 2018
Merged

Fix: Amend keyword-spacing to validate default keywords #11097

merged 2 commits into from Dec 3, 2018

Conversation

binury
Copy link
Contributor

@binury binury commented Nov 17, 2018

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

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
I updated the keyword-spacing rule definition to properly validate default keywords. At present, it mistakenly validates the export token rather than the accompanying default. I changed the handler to share checkSpacingForModuleDeclaration and amended that handler to include a special case for nodes with default export statements. This fixes: #11096

@jsf-clabot
Copy link

jsf-clabot commented Nov 17, 2018

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Nov 17, 2018
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, could you please also add a test, thanks!

@aladdin-add aladdin-add added bug ESLint is working incorrectly rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Nov 18, 2018
@binury
Copy link
Contributor Author

binury commented Nov 19, 2018

Sure thing; will do

@binury
Copy link
Contributor Author

binury commented Dec 2, 2018

@aladdin-add Added!

@platinumazure
Copy link
Member

platinumazure commented Dec 3, 2018

Has anyone (e.g., @aladdin-add) on the team had a chance to reproduce this issue? If so, we should label as "accepted".

(I tried to reproduce it just now, but the demo is unreliable on mobile.)

@aladdin-add
Copy link
Member

I'll check it later. @platinumazure

@aladdin-add aladdin-add added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 3, 2018
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I think this might need to be an "Update" commit since this could increase warnings for users. I'll take care of that on merge.

@platinumazure platinumazure merged commit 62fd2b9 into eslint:master Dec 3, 2018
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 2, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyword-spacing rule incorrectly applied to default keyword
4 participants