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

tools: Implement automatic fix for no-let-in-for-declaration #16642

Closed
wants to merge 1 commit into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Oct 31, 2017

Refs: #16636

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 31, 2017
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Hi @starkwang, thanks for working on this! Could you expand the test in test/parallel/test-eslint-no-let-in-for-declaration.js to test the fixer (output prop)?

See a sample of how it works here https://github.com/eslint/eslint/blob/master/tests/lib/rules/arrow-body-style.js

@starkwang
Copy link
Contributor Author

@apapirovski I've just added the test for fixer

@apapirovski
Copy link
Member

@tniessen
Copy link
Member

tniessen commented Nov 1, 2017

Basically LGTM, just note that automatic fixing can introduce bugs if another variable of the same name was declared within the function.

@joyeecheung
Copy link
Member

@tniessen I think we can rely on code reviews for that. If the author does not notice it themselves after running --fix, they probably would fix it the same way by hand anyway.

@apapirovski
Copy link
Member

Landed in 04ffa36

@apapirovski apapirovski closed this Nov 4, 2017
apapirovski pushed a commit that referenced this pull request Nov 4, 2017
PR-URL: #16642
Refs: #16636
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
PR-URL: nodejs#16642
Refs: nodejs#16636
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
MylesBorins pushed a commit that referenced this pull request Dec 19, 2017
PR-URL: #16642
Refs: #16636
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #16642
Refs: #16636
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants