Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
feat(eslint-plugin): [no-duplicate-enum-values] add rule#4833
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat(eslint-plugin): [no-duplicate-enum-values] add rule#4833
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nx-cloudbot commentedApr 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the PR,@aifreedom! 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 onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper day. |
netlifybot commentedApr 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site settings. |
codecovbot commentedApr 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #4833 +/- ##==========================================+ Coverage 91.77% 94.25% +2.48%========================================== Files 227 153 -74 Lines 10611 8305 -2306 Branches 3283 2703 -580 ==========================================- Hits 9738 7828 -1910+ Misses 591 263 -328+ Partials 282 214 -68
Flags with carried forward coverage won't be shown.Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The new rule and tests generally looks great, very nicely done@aifreedom!
Requesting changes for added test coverage. I can't view the Codecov coverage report at the moment to see why the file only has 80% 🙃 -- will take another look when more tests are added.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| return ( | ||
| node.type === AST_NODE_TYPES.Literal && typeof node.value === 'number' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[Nitpicking] Is there a need to separate these two functions out? They both check thatnode.type === AST_NODE_TYPES.Literal.
I'd suggest either making a single separate function:
functiongetExpressionLiteralValue(node:TSESTree.Expression){if(node.type===AST_NODE_TYPES.Literal){switch(typeofnode.value){// ...}}}...orinliningthembelow:```tsletvalue: string|number|undefined;if(node.type===AST_NODE_TYPES.Literal){// ...}
Just suggestions, feel free to ignore if you don't like either of these changes 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had to do this because typescript doesn't know the type ofmember.initializer without the util functions, and would complain when I cast itsvalue toString orNumber.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The coverage drop is a bit weird. It's an unrelated rule's coverage that dropped (no-magic-numbers). The coverage for the new rule is 100%.
Uh oh!
There was an error while loading.Please reload this page.
| return ( | ||
| node.type === AST_NODE_TYPES.Literal && typeof node.value === 'number' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had to do this because typescript doesn't know the type ofmember.initializer without the util functions, and would complain when I cast itsvalue toString orNumber.
…-enum-values.test.ts
93aaa38 to5484161CompareOh, no… I updated the branch using the “rebase” button and it seemed to have forced push the commits. Sorry about the inconvenience.@JoshuaKGoldberg you can review the last two commits which have the updates addressing your comments. |
Ha, no worries - thanks for the heads up! And yeah I don't know what's going on with Codecov. The service has been flaky off and on the last few... years. 🙃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fantastic, thanks so much@aifreedom!
This PR contains the following updates:| Package | Type | Update | Change ||---|---|---|---|| [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.21.0` -> `5.22.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.21.0/5.22.0) || [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.21.0` -> `5.22.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.21.0/5.22.0) |---### Release Notes<details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary>### [`v5.22.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#​5220-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5210v5220-2022-05-02)[Compare Source](typescript-eslint/typescript-eslint@v5.21.0...v5.22.0)##### Bug Fixes- **eslint-plugin:** \[comma-spacing] verify `nextToken` exists ([#​4868](typescript-eslint/typescript-eslint#4868)) ([23746f8](typescript-eslint/typescript-eslint@23746f8))##### Features- **eslint-plugin:** \[no-duplicate-enum-values] add rule ([#​4833](typescript-eslint/typescript-eslint#4833)) ([5899164](typescript-eslint/typescript-eslint@5899164))</details><details><summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary>### [`v5.22.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#​5220-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5210v5220-2022-05-02)[Compare Source](typescript-eslint/typescript-eslint@v5.21.0...v5.22.0)**Note:** Version bump only for package [@​typescript-eslint/parser](https://github.com/typescript-eslint/parser)</details>---### Configuration📅 **Schedule**: At any time (no schedule defined).🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.--- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.---This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).Co-authored-by: cabr2-bot <cabr2.help@gmail.com>Reviewed-on:https://codeberg.org/Calciumdibromid/CaBr2/pulls/1338Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
PR Checklist
Overview
This PR creates a new rule in
eslint-pluginto disallow duplicate enum values. Per discussion in the issue (#2693), in this version, it only checks enum members initialized with literals, and does not cover the ones without an initializer or initialized with an expression.