-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [no-magic-numbers] ignoreTypeIndexes option #4789
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
feat(eslint-plugin): [no-magic-numbers] ignoreTypeIndexes option #4789
Conversation
Add a new `ignoreTypeIndexes` option to the `no-magic-numbers` rule to allow magic numbers in type index access (eg. `Foo[4]`).
Thanks for the PR, @davecardwell! 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 #4789 +/- ##
==========================================
+ Coverage 93.96% 94.24% +0.28%
==========================================
Files 172 151 -21
Lines 9818 8230 -1588
Branches 3105 2676 -429
==========================================
- Hits 9225 7756 -1469
+ Misses 353 262 -91
+ Partials 240 212 -28
Flags with carried forward coverage won't be shown. Click here to find out more. |
(typeof node.value === 'number' || typeof node.value === 'bigint') && | ||
isAncestorTSIndexedAccessType(node) | ||
) { | ||
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.
FWIW, this looks good to me.
The only difference with my implementation is that I reported invalid nodes in this statement, similar to "Check if the node is a readonly class property". This way, we can return early which should be faster?
Also, good catch for the union types - this was something that I overlooked.
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.
@timdeschryver Thanks! I refactored things a little in d1e65be so if any of the four ignorable TypeScript rules are triggered it will report early. Is that better?
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.
Yea, this should work.
If we go into more details I would remove the else if (isAllowed === false)
statement to remove the indentation, and perhaps move the isAllowed
logic into its own method.
Keep in mind that I'm not a member of typescript-eslint 😅
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.
@timdeschryver Realized I never responded to this, sorry - and thank you for the feedback.
remove the
else if (isAllowed === false)
statement
I don’t think I can remove the isAllowed === false
case because it’s possible for isAllowed
to be undefined
, so I intend to explicitly check if it is true
or false
in that if/else if
block. If it is neither it will fall back to the base rule.
Perhaps I should move the final rules.Literal(node);
line of the function into an else
block to make that more explicit?
move the
isAllowed
logic into its own method
Looking at a few other rule definitions at random it does seem like they keep the core rule logic in the create
function, which makes sense to me. I’m not sure moving isAllowed
into its own function would improve readability, and feels like it would simply cause people to have to jump to look elsewhere for what is essentially the meat of the rule.
Feel free to push back if you (or @JoshuaKGoldberg?) disagree though, and I will happily take another look.
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.
No problem @davecardwell !
Thanks for the elaboration, that does make sense.
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.
Looks great so far, thanks! Just requesting some more unit tests.
type Foo = Bar[0]; | ||
type Baz = Parameters<Foo>[2]; | ||
``` | ||
|
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.
Heh, it's a pity this docs page doesn't use the newer tabbed format others do. No need to change in this PR though; I'm going to try to make docs a bit more automated soon.
{ | ||
code: "type Foo = Bar['baz'];", | ||
options: [{ ignoreTypeIndexes: false }], | ||
}, |
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.
A few edge cases we'll also want to test for:
type Foo = Bar[1 & number];
type Other = {
[0]: 3;
}
type Foo = {
[K in keyof Other]: `${K & number}`;
};
type Foo = {
[K in 0 | 1 | 2]: 0;
};
type Others = [ ['a'], ['b'] ];
type Foo = {
[K in keyof Others[0]]: Others[K];
};
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.
@JoshuaKGoldberg couple of quick questions on those:
type Foo = Bar[1 & number];
Sure, I check for union types so intersection should be covered too! Thanks.
type Other = { [0]: 3; } type Foo = { [K in keyof Other]: `${K & number}`; };
Since the 0
is a TSPropertySignature
, not a TSIndexedAccessType
, would you still want it to be covered by this new ignoreTypeIndexes
config? Seems like that should be a separate config?
Also, the 3
there should be an error, right? So perhaps string
or some other non-numeric literal would be more appropriate for this test case?
type Foo = { [K in 0 | 1 | 2]: 0; };
As above, TSPropertySignature
.
type Others = [ ['a'], ['b'] ]; type Foo = { [K in keyof Others[0]]: Others[K]; };
I’ll add a test to ensure the Others[0]
part is covered.
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.
Great questions!
check for union types so intersection should be covered too
Bar[1 & number]
is a bit different syntax from ${K & number}
. I think it'd be good to have an explicit test for both. The code right now might not differentiate, but it's ripe for future refactors to miss it.
Since the 0 is a
TSPropertySignature
, not aTSIndexedAccessType
, would you still want it to be covered by this newignoreTypeIndexes
config?
I'm not requesting changes to logic, just that it be covered. Agreed that they're different things -- the tests should validate that ignoreTypeIndexes
doesn't remove the complaint on it.
Also, the 3 there should be an error, right?
Yes. That's already covered though; I'm really more interested in the TSPropertySignature
.
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.
@JoshuaKGoldberg Thank you for clarifying. In af1ddb7 I added support for intersection types, nesting of union/intersection types, and the additional invalid test cases you suggested.
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.
Super, thanks for working on this @davecardwell! 🙌
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.19.0` -> `5.20.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.19.0/5.20.0) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.19.0` -> `5.20.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.19.0/5.20.0) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary> ### [`v5.20.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#​5200-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5190v5200-2022-04-18) [Compare Source](typescript-eslint/typescript-eslint@v5.19.0...v5.20.0) ##### Features - **eslint-plugin:** \[no-magic-numbers] ignoreTypeIndexes option ([#​4789](typescript-eslint/typescript-eslint#4789)) ([5e79451](typescript-eslint/typescript-eslint@5e79451)) </details> <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary> ### [`v5.20.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#​5200-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5190v5200-2022-04-18) [Compare Source](typescript-eslint/typescript-eslint@v5.19.0...v5.20.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/1312 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
Add a new
ignoreTypeIndexes
option to theno-magic-numbers
rule to allow magic numbers in type index access (eg.Foo[4]
).It sounds like @timdeschryver and I have both worked on this, so if this PR has major structural issues I’m happy to close this one in favor of his, or otherwise am happy to make any adjustments to see this one over the line.