-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Breaking: make radix
rule stricter
#12608
Conversation
Any chance this get merged? |
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.
Apologies for the delay in reviewing this. I'm personally not against this change, though I do think we either need to put it behind an option or release it in the next major release.
Can we update the documentation to make this behavior more transparent for end users?
Finally, we'll need a three more supporters from the team before we can mark this as accepted.
I'll champion this. |
I'll support this, but as @kaicataldo said, this either needs to be flagged as a breaking change or needs to be put behind an option. |
Thank you guys, any further work should I do? |
Nope! We just need one more 👍 for the team. Since we're now prepping our next release, I think it's fine to make this a breaking change. |
Nice enhancement! I've added my 👍 and marked this as accepted. |
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.
LGTM, thanks!
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.
Nice work on this! It'll be a nice enhancement. The implementation looks good as-is, though if you're willing to add it, reporting a separate message for an out-of-bounds radix might be helpful. For example, "Radix must be an integer between 2 and 36 inclusive" or something like that.
Can you also add a test with a non-integer radix so that we don't accidentally allow e.g. 10.5
in a future code change? LGTM once that's done.
radix
rule stricterradix
rule stricter
Changed message to Add tests, including |
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.
LGTM, thank you for contributing!
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.
It would be nice to add tests for boundary values: valid 2
and 36
and invalid 1
(37
is already tested).
@mdjermanovic Done 72ef36f |
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.
Thanks 👍
LGTM!
Thanks for contributing to ESLint! |
* Update: make `radix` rule stricter * style: remove extra blank line * Update message, more tests * Variable name & exponential notation test * test `10.0` * Add tests `1` `2` `36`
What is the purpose of this pull request? (put an "X" next to item)
[X] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
What rule do you want to change?
radix
Does this change cause the rule to produce more or fewer warnings?
more warnings
How will the change be implemented? (New option, new default behavior, etc.)?
new default behavior
Please provide some example code that this change will affect:
before,
radix
only check second param isnumber
, this PR only allow integer between 2 and 36 ref: parseInt SyntaxWhat does the rule currently do for this code?
What will the rule do after it's changed?
Is there anything you'd like reviewers to focus on?