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.
ematipico
left a comment
There was a problem hiding this comment.
Docs needs some rework to match our standards
| /// ```json,options | ||
| /// { | ||
| /// "options": { | ||
| /// "decimal": { | ||
| /// "minimumDigits": 7, | ||
| /// "groupLength": 3 | ||
| /// }, | ||
| /// "hexadecimal": { | ||
| /// "minimumDigits": 4, | ||
| /// "groupLength": 2 | ||
| /// } | ||
| /// } | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
To remove, as it doesn't follow the writing style for options
| /// ### `binary` | ||
| /// | ||
| /// Options for binary literals (e.g., `0b1010_0001`). | ||
| /// | ||
| /// - `minimumDigits` — Minimum number of digits required before adding separators. **Default: `0`** | ||
| /// - `groupLength` — Number of digits between separators. **Default: `4`** | ||
| /// | ||
| /// ### `octal` | ||
| /// | ||
| /// Options for octal literals (e.g., `0o1234_5670`). | ||
| /// | ||
| /// - `minimumDigits` — Minimum number of digits required before adding separators. **Default: `0`** | ||
| /// - `groupLength` — Number of digits between separators. **Default: `4`** | ||
| /// | ||
| /// ### `decimal` | ||
| /// | ||
| /// Options for decimal literals (e.g., `1_234_567`). | ||
| /// | ||
| /// - `minimumDigits` — Minimum number of digits required before adding separators. **Default: `5`** | ||
| /// - `groupLength` — Number of digits between separators. **Default: `3`** | ||
| /// |
There was a problem hiding this comment.
None of these options follows our pattern. Each option should have an option code block and a code block
There was a problem hiding this comment.
We don't want to overload users with repeated information, which was my original intention.
I have 2 ideas, but I'm kinda struggling to decide what would be better.
- list the top level options and then provide examples that use both sub-options,
### `decimal`
[description]
[default values for both sub options]
[example options block, both sub options]
[valid/invalid examples]
- document the sub-options object, and explain they go under the numeric literal type. so we would have entries like:
### `<numeric literal type>.minimumDigits`
Minimum number of digits required before adding separators. For example, with `minimumDigits` set to `5`, the number `1234` would not require separators, but `12345` would be expected to be formatted as `12_345`.
Default values for `minimumDigits` vary by numeric literal type:
- `binary`: `0`
- `octal`: `0`
- `decimal`: `5`
- `hexadecimal`: `0`
[valid/invalid examples, only one number type]
Which do you think is better? I'm personally leaning towards 2.
| pub struct NumericLiteralSeparatorOptions { | ||
| /// Minimum number of digits required before adding separators. | ||
| #[serde(skip_serializing_if = "Option::<_>::is_none")] | ||
| pub minimum_digits: Option<u8>, | ||
| /// Number of digits between separators. | ||
| #[serde(skip_serializing_if = "Option::<_>::is_none")] | ||
| pub group_length: Option<u8>, | ||
| } |
There was a problem hiding this comment.
The fact that both options are optional is weird, becase it means you can have an empty object e.g. "binary": {}. How would that work in this case?
There was a problem hiding this comment.
It would effectively be a no-op. They need to be Option so that merging configs makes sense.
I'll add validation at deserialization to make that impossible.
|
I just remembered that our parser doesn't even allow octal numbers. Will fix. |
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