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):prefer-consistent-enums rule#3891
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@brianconnoly! 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. |
7b44949 to33c5ac0Comparenx-cloudbot commentedSep 16, 2021 • 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.
codecovbot commentedSep 16, 2021 • 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 @@## master #3891 +/- ##==========================================+ Coverage 92.70% 93.51% +0.81%========================================== Files 329 150 -179 Lines 11534 8085 -3449 Branches 3257 2562 -695 ==========================================- Hits 10693 7561 -3132+ Misses 368 165 -203+ Partials 473 359 -114
Flags with carried forward coverage won't be shown.Click here to find out more.
|
33c5ac0 to3d3f634Compare3d3f634 toee959b1CompareLGTM in general, thanks for sending! Requesting changes on the error message and auto-fixer. |
| requiresTypeChecking: false, | ||
| }, | ||
| messages: { | ||
| nonConsistentEnum: `All enum members of {{ name }} must be same type (string, number, boolean, etc).`, |
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.
boolean
I'm under the impression TypeScript enums can't haveboolean typed values: onlystring ornumber.
| if (!memberType) { | ||
| return; | ||
| } |
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.
Indeed, this check isn't covered in tests. Perhaps it'd be good to have a test that includes an invalid enum value such as a boolean or array?
| /** | ||
| * If initializers types dont match — suggest change | ||
| */ | ||
| if (enumType !== memberType) { |
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.
Teeny nit: consider making this also return early, to reduce nesting later on? This function is pretty nested as-is.
| /** | |
| *Ifinitializerstypesdontmatch—suggestchange | |
| */ | |
| if(enumType!==memberType){ | |
| /** | |
| *Ifinitializerstypesmatch—don't suggest change | |
| */ | |
| if(enumType===memberType){ | |
| return; | |
| } |
(not a blocker IMO)
| node: member, | ||
| messageId: 'nonConsistentEnum', | ||
| data: { name: enumName }, | ||
| suggest: [ |
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'm a little -1 on suggesting such an unsafe change. In theory folks shouldn't rely on enum values but in practice it happens.eslint/eslint#7873
I'll defer to another reviewer's opinion if they feel strongly on it.
| create(context) { | ||
| const sourceCode = context.getSourceCode(); | ||
| function TSEnumDeclaration(node: TSESTree.TSEnumDeclaration): void { |
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.
Nit: out of curiosity, is there a reason to have this as a standalone function instead of an inline member of the returned object?
return { TSEnumDeclaration(node: TSESTree.TSEnumDeclaration): void { // ... }}Btw@brianconnoly we generally ask that PRs address an existing issue, to give folks a chance to discuss ideas & verify they'd be accepted before putting a lot of work into a PR. This one seems reasonable to me as a non-required rule so just maybe a heads up for next time? I'll defer to@bradzacher on whether this is a common enough use case to justify being in typescript-eslint core. |
Oh, this actually resolves#2694, I think! |
osdiab commentedJan 3, 2022
Should this be merged? |
@osdiab I'd requested changes that haven't yet been addressed.@brianconnoly do you still have the time to work on this? |
At this point the PR is now 6 months stale. Closing for housekeeping. |
Uh oh!
There was an error while loading.Please reload this page.
Added new rule —
prefer-consistent-enumsEnum members can have members of different types
Enums with string initializers can be used as object to iterate over them using
Object.keys/Object.valuesBut if member with number initializer will be added (or without one) — results of
Object.keys/Object.valueswill have additional auto generated itemsThis can lead to bugs, so this rule can prevent them by requiring enums to have consistent member types