chore(eslint-plugin): require underscore-delimited groups in numeric literals#3263
Conversation
🦋 Changeset detectedLatest commit: 44f4d1b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
b583f92 to
512438a
Compare
turadg
left a comment
There was a problem hiding this comment.
I'm not opposed to the rule but I think the config needs adjusting. Consider omitting the rule for test files and removing the hex requirement
|
|
||
| if (nonce === undefined) { | ||
| nonce = crypto.randomInt(0xffff_ffff); | ||
| nonce = crypto.randomInt(0xff_ff_ff_ff); |
There was a problem hiding this comment.
I find the pairs harder to read that quadruplets. Consider groups of four.
| if (code < 97) return code === 95; | ||
| if (code < 123) return true; | ||
| if (code <= 0xffff) | ||
| if (code <= 0xff_ff) |
There was a problem hiding this comment.
Another case in point of four-segment reading better
| withInterrupt(async ({ cancelled }) => { | ||
| await null; | ||
| const logCheckIntervalMs = ping !== undefined ? Number(ping) : 5_000; | ||
| const logCheckIntervalMs = ping !== undefined ? Number(ping) : 5000; |
There was a problem hiding this comment.
Why prefer no underscore for thousands?
| t.is( | ||
| parseBigint('123456789012345678901234567890'), | ||
| 123456789012345678901234567890n, | ||
| 123_456_789_012_345_678_901_234_567_890n, |
There was a problem hiding this comment.
This is worse. The reader wants to verify the numeral sequence matches the above. Consider suppressing the rule on this file
| // Deterministic PRNG, same seed shape as other Endo fuzz tests. | ||
| const defaultSeed = [0xb0b5c0ff, 0xeefacade, 0xb0b5c0ff, 0xeefacade]; | ||
| const defaultSeed = [ | ||
| 0xb0_b5_c0_ff, 0xee_fa_ca_de, 0xb0_b5_c0_ff, 0xee_fa_ca_de, |
There was a problem hiding this comment.
These visually break the mnemonic. Please suppress this line or file
| // Deterministic PRNG, same seed shape as other Endo fuzz tests. | ||
| const defaultSeed = [0xb0b5c0ff, 0xeefacade, 0xb0b5c0ff, 0xeefacade]; | ||
| const defaultSeed = [ | ||
| 0xb0_b5_c0_ff, 0xee_fa_ca_de, 0xb0_b5_c0_ff, 0xee_fa_ca_de, |
| let bits = BigInt(`0x${getSuffix(encoded, skip + 1)}`); | ||
| if (encoded.charAt(skip + 1) < '8') { | ||
| bits ^= 0xffffffffffffffffn; | ||
| bits ^= 0xff_ff_ff_ff_ff_ff_ff_ffn; |
There was a problem hiding this comment.
What's salient here is the length and this is harder to count than groups of four.
| [9007199254740993n, { '@qclass': 'bigint', digits: '9007199254740993' }], | ||
| [9_007_199_254_740_993n, { '@qclass': 'bigint', digits: '9007199254740993' }], | ||
|
|
| // See https://github.com/ocapn/ocapn/blob/main/draft-specifications/Model.md#string | ||
| const INVALID_STRING_CHARS_START = 0xd800; | ||
| const INVALID_STRING_CHARS_END = 0xdfff; | ||
| const INVALID_STRING_CHARS_START = 0xd8_00; |
There was a problem hiding this comment.
Another case for fours: the comment above is in fours. The literal should match
|
Mirror of endojs/endo-but-for-bots#244 (head 512438a). |
…s/endo#3263 (Shape 2; numeric-separators; CONFLICTING)
512438a to
46de187
Compare
… recompute, CONFLICTING→MERGEABLE); message: boatman → steward upstream cross-link
…endo#3263 (force-push; CONFLICTING->MERGEABLE; numeric-separators)
46de187 to
4d039c3
Compare
…o#3263 (clear CONFLICTING; numeric-separators)
4d039c3 to
eef8f2f
Compare
…o#3263 (base-freshen; content unchanged)
eef8f2f to
0e861ff
Compare
…d bots#244 (content unchanged; MERGEABLE)
| { | ||
| onlyIfContainsSeparator: false, | ||
| number: { minimumDigits: 5, groupLength: 3 }, | ||
| binary: { minimumDigits: 0, groupLength: 4 }, | ||
| octal: { minimumDigits: 0, groupLength: 4 }, | ||
| hexadecimal: { minimumDigits: 0, groupLength: 4 }, | ||
| }, |
There was a problem hiding this comment.
@turadg The bulk of your feedback is addressed here, and with some one-off exceptions where the rules are disabled to make other representations more obviously equivalent.
| */ | ||
| /** @this {Compartment} */ | ||
| /** | ||
| * @param {...any} args |
There was a problem hiding this comment.
Thank you for catching this.
0e861ff to
44f4d1b
Compare
Description
Adds
eslint-plugin-unicorn'snumeric-separators-stylerule to the@endo/internalESLint preset, enforcing underscore-delimited groupings on numeric literals. Decimal numbers of five or more digits group every three digits (1_234_567); hexadecimal, binary, and octal literals group by the rule's conventional widths (hex in twos, binary and octal in fours). The rule is autofixable, soeslint --fixapplied the migration across the codebase in a separate commit. A final commit reflows seven autofix-touched files that Prettier wanted to re-wrap once the separators pushed a few expressions past the print-width threshold.Security Considerations
None. Numeric separators are a syntactic transformation that does not change the runtime value of any literal.
Scaling Considerations
None.
Documentation Considerations
The
@endo/eslint-pluginchangeset notes that consumers ofplugin:@endo/internalwill see lint errors on numeric literals that violate the rule and must addeslint-plugin-unicornto their devDependencies to satisfy the new peer dependency.eslint --fixrewrites violations automatically, so the migration cost downstream is one autofix run per consumer.Testing Considerations
yarn lintis the test surface. The pre-PR run on this branch reports zero ESLint errors and zero Prettier warnings on the files this migration touches. Two pre-existing ESLint warnings onmasterremain unaddressed (inevasive-transform/src/index.jsandses/src/compartment.js); they predate this change and are out of scope here.Compatibility Considerations
The rule strips separators from decimal literals below the minimum-digits threshold (
1_000becomes1000). This matches the rule's defaults and is consistent with the convention that separators communicate magnitude only when the number is long enough to benefit from grouping.Upgrade Considerations
None for production systems. Downstream consumers of
@endo/eslint-pluginwill need to installeslint-plugin-unicorn@^56.0.1(a new peer dependency of the plugin); the published changeset describes the requirement.