feat(eslint-plugin): prefer-consistent-enums rule
#3891
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 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. |
7b44949 to
33c5ac0
Compare
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 to
3d3f634
Compare
3d3f634 to
ee959b1
Compare
|
LGTM 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.
boolean
I'm under the impression TypeScript enums can't have boolean typed values: only string or number.
|
|
||
| if (!memberType) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
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.
Teeny nit: consider making this also return early, to reduce nesting later on? This function is pretty nested as-is.
| /** | |
| * If initializers types dont match — suggest change | |
| */ | |
| if (enumType !== memberType) { | |
| /** | |
| * If initializers types match — 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.
🤔 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.
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! |
|
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. |
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