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
Update: support numeric separators (refs #13568) #13581
Conversation
I suppose that would be in a separate PR #13568 (comment) |
const DECIMAL_INTEGER_PATTERN = /^(0|[1-9]\d*)$/u; | ||
const DECIMAL_INTEGER_PATTERN = /^(0|[1-9](?:_?\d)*)$/u; |
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.
This actually had a bug: it doesn't recognize NonOctalDecimalIntegerLiteral literals, like 08
.
I think we can fix this later since it's an existing edge-case bug that isn't related to numeric separators and requires a non-trivial change.
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.
Mind making an issue?
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.
Opened #13588
1c10aa7
to
785a7cc
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.
LGTM. The additional tests you added are very thorough.
}, | ||
{ | ||
|
||
// this would be indeed the same as `0x0_0`, but there's no need to autofix this edge case that looks more like a mistake. |
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.
Agreed that we don't need to handle this, and I appreciate that you noticed the edge case exists and added an explicit test for it 👍
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!
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[X] Other, please explain:
refs #13568
Adds support for numeric separators (along with PR #13574).
What changes did you make? (Give an overview)
astUtils.isDecimalInteger
andisDecimalIntegerNumericToken
. Also added tests for these helpers and some tests for rules that are using them.prefer-numeric-literals
andquote-props
rules.In total, this is one line of code + tests and comments.
Is there anything you'd like reviewers to focus on?
package.json
should be updated before merging.