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): add no-magic-numbers rule #373
feat(eslint-plugin): add no-magic-numbers rule #373
Conversation
24a7f3e
to
8e1935d
Compare
Codecov Report
@@ Coverage Diff @@
## master #373 +/- ##
==========================================
- Coverage 95.76% 95.74% -0.03%
==========================================
Files 78 79 +1
Lines 3493 3523 +30
Branches 963 975 +12
==========================================
+ Hits 3345 3373 +28
Misses 53 53
- Partials 95 97 +2
|
35c1237
to
ac78113
Compare
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.
Code itself LGTM. Few (very) minor nits.
Haven't gotten around to doing it for the existing rules yet, but one thing we want to do with extension rules is to stop copying the documentation from eslint
.
Whilst it does force users to click through for more documentation, it saves us from having to track changes on eslint master to update our docs.
Could you please change the docs to something akin to following structure:
- Header
- Rule details
- The description
- Link to the existing rule documentation.
- Rule Changes
- Added Options.
Thanks for your contribution!
Thanks @bradzacher for the positive feedback. Regarding the docs format, I agree that not duplicating the core docs is a good move; so I've updated the docs with what I believe you were recommending. Hope I've understood correctly. |
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.
everything looks good, one last fix.
d83bc8e
to
c188361
Compare
Extends the
no-magic-numbers
rule from ESlint core with an additional option (ignoreNumericLiteralTypes
) to ignore Typescript numeric literal types when checking for magic numbers.Fixes #99
The new option allows for type definitions such as the ones listed below to be considered valid.
Currently, the base
no-magic-numbers
rule currently considers these type definitions as errors, despite there being no valid way (at least that I'm aware of) to define numeric literal types that won't violate the magic numbers rule.Examples of allowed code
Please note that this is my first time contributing a TS rule, so if I've missed anything or if there is a preferred way to implement this, suggestions are welcome.
Also note, unsure if this should be a "recommended" rule or not.