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

Breaking: make no-redeclare stricter (fixes #11370, fixes #11405) #11509

Merged
merged 17 commits into from Apr 25, 2019

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Mar 15, 2019

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

[X] Changes an existing rule
[X] Add something to the core

What changes did you make? (Give an overview)

This PR makes no-redeclare rule stricter.

And some chore:

  • Removes the accesses to parserOptions from no-redeclare rule. The parserOptions is not safe because each parser has different options.
  • Moves a utility that determines the location of a variable name in /*globals*/ comments to ast-utils because it's shared with no-unused-vars rule and no-redeclare rule.
  • Adds several tests for lexical bindings into no-redeclare rule along with a loose parser that doesn't throw syntax errors on redeclaration of lexical bindings.
  • Makes eslint-plugin/test-case-property-ordering rule's option more reasonable. (moved to Chore: make test-case-property-ordering reasonable #11511)

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

  • Should no-redeclare rule handle lexical bindings still? Since acorn 5.0.0, it throws syntax errors by such a redeclaration.

@mysticatea mysticatea added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible do not merge This pull request should not be merged yet labels Mar 15, 2019
@mysticatea mysticatea added this to Implemented, pending review in v6.0.0 Mar 15, 2019
Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I haven't gone through the implementation thoroughly yet, but I left a few suggestions.

docs/user-guide/migrating-to-6.0.0.md Outdated Show resolved Hide resolved
lib/rules/no-redeclare.js Outdated Show resolved Hide resolved
lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
docs/developer-guide/working-with-rules.md Outdated Show resolved Hide resolved
@platinumazure platinumazure self-requested a review March 15, 2019 19:47
# Conflicts:
#	tests/lib/rules/no-redeclare.js
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.

Left a few small suggestions.

I'm a little leery about consuming acorn in ESLint, but there's probably not much choice here. Part of me wonders if we should expose a loose parsing mode in espree and just use that, but maybe that would just be a support headache as people ask us about issues involving loose espree...

docs/developer-guide/working-with-rules.md Outdated Show resolved Hide resolved
lib/util/ast-utils.js Outdated Show resolved Hide resolved
tests/lib/rules/no-redeclare.js Outdated Show resolved Hide resolved
platinumazure and others added 3 commits March 29, 2019 09:32
Co-Authored-By: mysticatea <star.ctor@gmail.com>
Co-Authored-By: mysticatea <star.ctor@gmail.com>
@mysticatea mysticatea removed the do not merge This pull request should not be merged yet label Apr 2, 2019
# Conflicts:
#	lib/config/config-ops.js
#	lib/linter.js
#	tests/lib/config/config-ops.js
@mysticatea mysticatea moved this from Implemented, pending review to Ready to merge in v6.0.0 Apr 8, 2019
@mysticatea mysticatea moved this from Ready to merge to Implemented, pending review in v6.0.0 Apr 8, 2019
@not-an-aardvark not-an-aardvark dismissed their stale review April 10, 2019 18:45

I'm not sure when I'll have time to review this thoroughly, but my previous review has been addressed.

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! Sorry for the delay.

@mysticatea
Copy link
Member Author

Thank you very much!

@mysticatea mysticatea merged commit 20364cc into master Apr 25, 2019
v6.0.0 automation moved this from Implemented, pending review to Done Apr 25, 2019
@mysticatea mysticatea deleted the no-redeclare/issue11370and11405 branch April 25, 2019 09:28
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Oct 23, 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 Oct 23, 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 breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
No open projects
v6.0.0
  
Done
3 participants