feat(useNumericSeparators): add options for minimum digits and group length#9571
feat(useNumericSeparators): add options for minimum digits and group length#9571
Conversation
🦋 Changeset detectedLatest commit: bb3efae The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
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 |
Merging this PR will not alter performance
Comparing 
Footnotes
|
WalkthroughThis change introduces configurable options for the Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_rule_options/src/use_numeric_separators.rs (1)
56-109: Consider reducing duplication in accessor methods.Each accessor follows the same pattern. You could simplify by calling
resolve()unconditionally on an unwrapped-or-default reference:♻️ Optional refactor
/// Returns `(min_digits, group_length)` for binary literals. pub fn binary(&self) -> (usize, usize) { - match &self.binary { - Some(opts) => opts.resolve( - Self::DEFAULT_BINARY_MIN_DIGITS, - Self::DEFAULT_BINARY_GROUP_LENGTH, - ), - None => ( - Self::DEFAULT_BINARY_MIN_DIGITS as usize, - Self::DEFAULT_BINARY_GROUP_LENGTH as usize, - ), - } + self.binary + .as_ref() + .unwrap_or(&NumericLiteralSeparatorOptions::default()) + .resolve(Self::DEFAULT_BINARY_MIN_DIGITS, Self::DEFAULT_BINARY_GROUP_LENGTH) }That said, the explicit
matchis perfectly readable — feel free to keep it if you prefer the clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_rule_options/src/use_numeric_separators.rs` around lines 56 - 109, The four accessor methods binary, octal, decimal, and hexadecimal duplicate the same match/None pattern; refactor by centralizing the logic so each just calls a single resolver. Add a small helper (e.g., resolve_numeric or resolve_or_default) that accepts the Option (the fields used by binary/octal/decimal/hexadecimal), the corresponding DEFAULT_*_MIN_DIGITS and DEFAULT_*_GROUP_LENGTH values, and returns (usize, usize) by calling opts.resolve(...) when Some or using the defaults when None; then have binary, octal, decimal, and hexadecimal call that helper passing their respective DEFAULT_* constants and the field so the resolve() invocation is not duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_cli/src/execute/migrate/eslint_unicorn.rs`:
- Around line 84-98: The migration currently drops the ESLint
onlyIfContainsSeparator option silently; update the
NumericSeparatorsStyleOptions and EslintNumericSeparatorTypeOptions structs to
include an Option<bool> field named only_if_contains_separator (or similar) to
capture that flag, then modify the migration path to detect when
only_if_contains_separator is Some(true/false) and emit a migration notice
explaining Biome doesn't support it and the behavior will be different;
reference the structs NumericSeparatorsStyleOptions and
EslintNumericSeparatorTypeOptions and ensure the migration notice is produced
wherever these structs are parsed/converted so users see the delta.
---
Nitpick comments:
In `@crates/biome_rule_options/src/use_numeric_separators.rs`:
- Around line 56-109: The four accessor methods binary, octal, decimal, and
hexadecimal duplicate the same match/None pattern; refactor by centralizing the
logic so each just calls a single resolver. Add a small helper (e.g.,
resolve_numeric or resolve_or_default) that accepts the Option (the fields used
by binary/octal/decimal/hexadecimal), the corresponding DEFAULT_*_MIN_DIGITS and
DEFAULT_*_GROUP_LENGTH values, and returns (usize, usize) by calling
opts.resolve(...) when Some or using the defaults when None; then have binary,
octal, decimal, and hexadecimal call that helper passing their respective
DEFAULT_* constants and the field so the resolve() invocation is not duplicated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d550d887-f987-44fc-909a-30ad207630d7
⛔ Files ignored due to path filters (4)
crates/biome_js_analyze/tests/specs/style/useNumericSeparators/invalidCustomOptions.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/style/useNumericSeparators/validCustomOptions.js.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (10)
.changeset/use-numeric-separators-options.mdcrates/biome_cli/src/execute/migrate/eslint_eslint.rscrates/biome_cli/src/execute/migrate/eslint_to_biome.rscrates/biome_cli/src/execute/migrate/eslint_unicorn.rscrates/biome_js_analyze/src/lint/style/use_numeric_separators.rscrates/biome_js_analyze/tests/specs/style/useNumericSeparators/invalidCustomOptions.jscrates/biome_js_analyze/tests/specs/style/useNumericSeparators/invalidCustomOptions.options.jsoncrates/biome_js_analyze/tests/specs/style/useNumericSeparators/validCustomOptions.jscrates/biome_js_analyze/tests/specs/style/useNumericSeparators/validCustomOptions.options.jsoncrates/biome_rule_options/src/use_numeric_separators.rs
| /// Note: ESLint uses `number` for decimal literals, while Biome uses `decimal`. | ||
| /// The `onlyIfContainsSeparator` option is not supported and is ignored. | ||
| #[derive(Clone, Debug, Default, Deserializable)] | ||
| pub(crate) struct NumericSeparatorsStyleOptions { | ||
| number: EslintNumericSeparatorTypeOptions, | ||
| binary: EslintNumericSeparatorTypeOptions, | ||
| octal: EslintNumericSeparatorTypeOptions, | ||
| hexadecimal: EslintNumericSeparatorTypeOptions, | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Default, Deserializable)] | ||
| pub(crate) struct EslintNumericSeparatorTypeOptions { | ||
| minimum_digits: Option<u8>, | ||
| group_length: Option<u8>, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In eslint-plugin-unicorn's numeric-separators-stylerule, isonlyIfContainsSeparator supported both at top-level and per numeric type?
💡 Result:
Yes. The rule supports onlyIfContainsSeparator both:
- Top-level (
{ onlyIfContainsSeparator: true }), applying as the default for all numeric types, and - Per numeric type (
binary/octal/hexadecimal/number), overriding the top-level value for that type. The docs state this explicitly, and the implementation merges top-level into each type while allowing the per-type option to override it. (raw.githubusercontent.com)
Citations:
🏁 Script executed:
cat -n crates/biome_cli/src/execute/migrate/eslint_unicorn.rs | sed -n '111,130p'Repository: biomejs/biome
Length of output: 837
🏁 Script executed:
rg -t rs "struct.*UseNumericSeparatorsOptions" -A 15Repository: biomejs/biome
Length of output: 83
🏁 Script executed:
rg "struct.*UseNumericSeparatorsOptions" -A 15Repository: biomejs/biome
Length of output: 1731
🏁 Script executed:
rg "struct.*NumericLiteralSeparatorOptions" -A 10Repository: biomejs/biome
Length of output: 1137
Capture onlyIfContainsSeparator for migration notice—Biome doesn't support it yet, but silently dropping it can surprise users.
ESLint's rule supports this option at both top-level and per-type, but Biome's NumericLiteralSeparatorOptions doesn't include it. Rather than silently dropping it (lines 84–98 and 111–130), capture it in the structs and emit a migration notice when present, so the behaviour delta is explicit.
Suggested capture
pub(crate) struct NumericSeparatorsStyleOptions {
+ only_if_contains_separator: Option<bool>,
number: EslintNumericSeparatorTypeOptions,
binary: EslintNumericSeparatorTypeOptions,
octal: EslintNumericSeparatorTypeOptions,
hexadecimal: EslintNumericSeparatorTypeOptions,
}
pub(crate) struct EslintNumericSeparatorTypeOptions {
+ only_if_contains_separator: Option<bool>,
minimum_digits: Option<u8>,
group_length: Option<u8>,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Note: ESLint uses `number` for decimal literals, while Biome uses `decimal`. | |
| /// The `onlyIfContainsSeparator` option is not supported and is ignored. | |
| #[derive(Clone, Debug, Default, Deserializable)] | |
| pub(crate) struct NumericSeparatorsStyleOptions { | |
| number: EslintNumericSeparatorTypeOptions, | |
| binary: EslintNumericSeparatorTypeOptions, | |
| octal: EslintNumericSeparatorTypeOptions, | |
| hexadecimal: EslintNumericSeparatorTypeOptions, | |
| } | |
| #[derive(Clone, Debug, Default, Deserializable)] | |
| pub(crate) struct EslintNumericSeparatorTypeOptions { | |
| minimum_digits: Option<u8>, | |
| group_length: Option<u8>, | |
| } | |
| /// Note: ESLint uses `number` for decimal literals, while Biome uses `decimal`. | |
| /// The `onlyIfContainsSeparator` option is not supported and is ignored. | |
| #[derive(Clone, Debug, Default, Deserializable)] | |
| pub(crate) struct NumericSeparatorsStyleOptions { | |
| only_if_contains_separator: Option<bool>, | |
| number: EslintNumericSeparatorTypeOptions, | |
| binary: EslintNumericSeparatorTypeOptions, | |
| octal: EslintNumericSeparatorTypeOptions, | |
| hexadecimal: EslintNumericSeparatorTypeOptions, | |
| } | |
| #[derive(Clone, Debug, Default, Deserializable)] | |
| pub(crate) struct EslintNumericSeparatorTypeOptions { | |
| only_if_contains_separator: Option<bool>, | |
| minimum_digits: Option<u8>, | |
| group_length: Option<u8>, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_cli/src/execute/migrate/eslint_unicorn.rs` around lines 84 - 98,
The migration currently drops the ESLint onlyIfContainsSeparator option
silently; update the NumericSeparatorsStyleOptions and
EslintNumericSeparatorTypeOptions structs to include an Option<bool> field named
only_if_contains_separator (or similar) to capture that flag, then modify the
migration path to detect when only_if_contains_separator is Some(true/false) and
emit a migration notice explaining Biome doesn't support it and the behavior
will be different; reference the structs NumericSeparatorsStyleOptions and
EslintNumericSeparatorTypeOptions and ensure the migration notice is produced
wherever these structs are parsed/converted so users see the delta.
Summary
I realize the way the options are documented is a little different than what we usually do. The agent came up with it, and tbh I think the more concise summary is more preferable than having valid and invalid cases for all 4 * 2 options.
Generated by opus 4.6
discussion: #9316
Test Plan
snapshots
Docs