feat(eslint-plugin): [no-duplicate-enum-values] add rule #4833
Conversation
|
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 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
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.
| return ( | ||
| node.type === AST_NODE_TYPES.Literal && typeof node.value === 'number' | ||
| ); | ||
| } |
There was a problem hiding this comment.
[Nitpicking] Is there a need to separate these two functions out? They both check that node.type === AST_NODE_TYPES.Literal.
I'd suggest either making a single separate function:
function getExpressionLiteralValue(node: TSESTree.Expression) {
if (node.type === AST_NODE_TYPES.Literal) {
switch (typeof node.value) {
// ...
}
}
}
...or inlining them below:
```ts
let value: 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.
I had to do this because typescript doesn't know the type of member.initializer without the util functions, and would complain when I cast its value to String or Number.
| return ( | ||
| node.type === AST_NODE_TYPES.Literal && typeof node.value === 'number' | ||
| ); | ||
| } |
There was a problem hiding this comment.
I had to do this because typescript doesn't know the type of member.initializer without the util functions, and would complain when I cast its value to String or Number.
…-enum-values.test.ts
93aaa38 to
5484161
Compare
|
Oh, 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. 🙃 |
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/1338 Reviewed-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.