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

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 jsf-clabot commented Nov 17, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@aladdin-add aladdin-add left a comment

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

@binury
Copy link
Contributor Author

@binury binury commented Nov 19, 2018

Sure thing; will do

@binury
Copy link
Contributor Author

@binury binury commented Dec 2, 2018

@aladdin-add Added!

@platinumazure
Copy link
Member

@platinumazure 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

@aladdin-add aladdin-add commented Dec 3, 2018

I'll check it later. @platinumazure

Copy link
Member

@aladdin-add aladdin-add left a comment

LGTM, thanks!

Copy link
Member

@platinumazure platinumazure left a comment

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
5 checks passed
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants