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

Jump to bottom

feat(eslint-plugin): add rule prefer-literal-enum-member #1898

Merged
merged 12 commits into from Jul 6, 2020
Merged

feat(eslint-plugin): add rule prefer-literal-enum-member #1898

merged 12 commits into from Jul 6, 2020

Conversation

oigewan
Copy link
Contributor

@oigewan oigewan commented Apr 14, 2020

Issue:
Several users have run into an unexpected behavior for the no-shadow rule. Specifically, they do not expect that an enum member name will end up shadowing other variables with the same name in that scope, and they consider it a false positive. An example can be seen here #325 (comment)

Changes:
This PR adds a rule that aims to prevent users from assigning a variable to an enum member, which could help alleviate the aforementioned issue. If accepted, the no-shadow rule can be updated with an option that will ignore enum members with documentation recommending that this rule be enabled if said option is also enabled.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @oigewan!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@oigewan oigewan mentioned this pull request Apr 14, 2020
Closed
@codecov
Copy link

codecov bot commented Apr 14, 2020 •
edited

Codecov Report

Merging #1898 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1898   +/-   ##
=======================================
  Coverage   93.09%   93.10%           
=======================================
  Files         282      283    +1     
  Lines        9014     9021    +7     
  Branches     2463     2465    +2     
=======================================
+ Hits         8392     8399    +7     
  Misses        301      301           
  Partials      321      321
Flag Coverage Δ
#unittest 93.10% <100.00%> (+<0.01%) ⬆️
Impacted Files Coverage Δ
packages/eslint-plugin/src/configs/all.ts 100.00% <ø> (ø)
...int-plugin/src/rules/prefer-literal-enum-member.ts 100.00% <100.00%> (ø)

@bradzacher bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Apr 14, 2020
bradzacher
bradzacher requested changes May 17, 2020
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

a few comments.
Thanks for your work!

packages/eslint-plugin/src/rules/no-identifier-enum-member.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/tests/rules/no-identifier-enum-member.test.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/tests/rules/no-identifier-enum-member.test.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/no-identifier-enum-member.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/no-identifier-enum-member.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/no-identifier-enum-member.md Outdated Show resolved Hide resolved
packages/eslint-plugin/docs/rules/no-identifier-enum-member.md Outdated Show resolved Hide resolved
@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label May 17, 2020
@oigewan oigewan changed the title feat(eslint-plugin): add no-identifier-enum-member rule feat(eslint-plugin): add prefer-literal-enum-member rule Jun 17, 2020
@oigewan
Copy link
Contributor Author

oigewan commented Jun 17, 2020

@bradzacher PR updated and checks have passed.

@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 17, 2020
bradzacher
bradzacher requested changes Jun 19, 2020
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

mostly LGTM - a few comments
I think this version of the rule is much better than before - super simple and clear.
Thanks!

packages/eslint-plugin/tests/rules/prefer-literal-enum-member.test.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/prefer-literal-enum-member.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/prefer-literal-enum-member.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/prefer-literal-enum-member.ts Outdated Show resolved Hide resolved
@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Jun 19, 2020
@oigewan
Copy link
Contributor Author

oigewan commented Jun 25, 2020

@bradzacher I updated this PR with most of your suggestions. I didn't implement the selector in the way that suggested, but I think that my update addresses the code readability issue. Let me know if I'm missing something.

@oigewan oigewan requested a review from bradzacher June 26, 2020 18:45
@bradzacher bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Jun 26, 2020
bradzacher
bradzacher approved these changes Jul 6, 2020
View reviewed changes
Copy link
Member

@bradzacher bradzacher 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 for this!

@bradzacher bradzacher changed the title feat(eslint-plugin): add prefer-literal-enum-member rule feat(eslint-plugin): add rule prefer-literal-enum-member Jul 6, 2020
@bradzacher bradzacher merged commit fe2b2ec into typescript-eslint:master Jul 6, 2020
@oigewan oigewan deleted the enum-assign branch July 6, 2020 17:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bradzacher bradzacher bradzacher approved these changes

Assignees
No one assigned
Labels
enhancement: new plugin rule New rule request for eslint-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@oigewan @bradzacher