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
infra(unicorn): numeric-separators-style #2815
base: next
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Not sure about this one. Sometimes it looks useful, at other times the numbers aren't actual numbers... |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #2815 +/- ##
==========================================
- Coverage 99.95% 99.95% -0.01%
==========================================
Files 2977 2977
Lines 215421 215421
Branches 948 947 -1
==========================================
- Hits 215327 215326 -1
- Misses 94 95 +1
|
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.
Sill thinking that it looks weird for digits <= 5 🤷
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.
I'm not convinced about the need for this rule. I think we could leave it up to authors to use separators when appropriate.
@@ -171,7 +171,7 @@ ${examples}`; | |||
|
|||
// This only checks whether the whole method is deprecated or not | |||
// It does not check whether the method is deprecated for a specific set of arguments | |||
it('verify @deprecated tag', { timeout: 30000 }, async () => { | |||
it('verify @deprecated tag', { timeout: 30_000 }, async () => { |
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.
Some cases like this are an improvement
['IL', 60001, 62999], | ||
['GA', 30001, 31999], | ||
['WA', 98001, 99403], | ||
['IL', 60_001, 62_999], |
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.
But some like this are worse
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.
Curious: Could you please give some insights why you consider this worse than the case above and the original?
Is it because the resulting value doesn't have it/isn't really a number?
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.
Yes
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's not Beverly Hills 90_210
I've evaluated the proposed rule again and it has become apparent for me that its implementation could generally enhance the uniformity of "magic number" definitions within a codebase. However, our repository's unique context, where numeric values often carry specific meanings, may be compromised by the adoption of numeric separator syntax. This concern was previously expressed by @matthewmayer (see here) as well. While it is possible to configure the rule to enforce separators only where they currently exist, thereby standardizing their usage (e.g., in groups of three), this approach contradicts the rationale for introducing the rule in the first place. Given these considerations, I propose that we opt not to include this rule in our ruleset. |
Team Decision
|
Ref: #2439
Enables the
unicorn/numeric-separators-style
lint rule.