feat(formatter): add delimiterSpacing option#8966
feat(formatter): add delimiterSpacing option#8966luisherranz wants to merge 21 commits intobiomejs:nextfrom
delimiterSpacing option#8966Conversation
chore(formatter): add changeset
🦋 Changeset detectedLatest commit: 40d1732 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new formatter option Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can get early access to new features in CodeRabbit.Enable the |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/biome_js_formatter/src/js/bindings/parameters.rs (1)
98-115:⚠️ Potential issue | 🟠 MajorAdd a trailing space before
)for Default layout whendelimiterSpacingis on.
Default layout currently inserts only the leading space, so single-line params become( a, b)rather than( a, b ).Suggested fix
write!( f, [soft_block_indent_with_maybe_space( &FormatJsAnyParameterList::with_layout(&list, ParameterLayout::Default,), should_insert_space_around_delimiters )] )?; if !parentheses_not_needed { + if should_insert_space_around_delimiters { + write!(f, [soft_line_break_or_space()])?; + } write!(f, [r_paren_token.format()])?; } else {crates/biome_js_formatter/src/lib.rs (1)
376-405:⚠️ Potential issue | 🟡 MinorReplace hard
space()withsoft_line_break_or_space()around inserted parentheses.When delimiter spacing wraps content across lines, hard spaces leave trailing whitespace. Use
soft_line_break_or_space()instead, which collapses intelligently when the group breaks—it's already the standard pattern throughout the formatter for this scenario.Fix
if needs_parentheses { write!(f, [token("(")])?; if should_insert_space { - write!(f, [space()])?; + write!(f, [soft_line_break_or_space()])?; } } if let Some(range) = self.embedded_node_range(node, f) { // Tokens that belong to embedded nodes are formatted later on, // so we track them, even though they aren't formatted now during this pass. let state = f.state_mut(); for token in node.syntax().tokens() { state.track_token(&token); } f.write_elements(vec![ FormatElement::Tag(StartEmbedded(range)), FormatElement::Tag(EndEmbedded), ])?; } else { self.fmt_fields(node, f)?; } if needs_parentheses { if should_insert_space { - write!(f, [space()])?; + write!(f, [soft_line_break_or_space()])?; } write!(f, [token(")")])?; }crates/biome_js_formatter/src/js/declarations/catch_declaration.rs (1)
40-48:⚠️ Potential issue | 🟡 MinorApply delimiter spacing consistently in the breaking path.
The breaking path (lines 40–48) doesn't apply
delimiter_spacing, whilst the non-breaking path does. Other formatters in the codebase usesoft_block_indent_with_maybe_spacefor this pattern. The fix is straightforward:Suggested change
if leading_comment_with_break || trailing_comment_with_break { write!( f, [ l_paren_token.format(), - soft_block_indent(&format_args![binding.format(), type_annotation.format()]), + soft_block_indent_with_maybe_space( + &format_args![binding.format(), type_annotation.format()], + should_insert_space_around_delimiters + ), r_paren_token.format() ] )crates/biome_js_formatter/src/js/expressions/call_arguments.rs (1)
848-879:⚠️ Potential issue | 🟠 MajorInline call‑argument layout misses the closing‑space.
When the group stays on one line,soft_block_indent_with_maybe_spaceinserts only the leading space after(; the closing)stays tight. Add a conditionalif_group_fits_on_line(&space())beforeself.r_parenwhenshould_insert_spaceis true.Suggested fix
- soft_block_indent_with_maybe_space( + soft_block_indent_with_maybe_space( &format_with(|f| { for (index, entry) in self.args.iter().enumerate() { if index > 0 { match entry.leading_lines() { 0 | 1 => write!(f, [soft_line_break_or_space()])?, _ => write!(f, [empty_line()])?, } } write!(f, [entry])?; } if !is_inside_import { write!(f, [FormatTrailingCommas::All])?; } Ok(()) }), self.should_insert_space - ), + ), + self.should_insert_space.then_some(if_group_fits_on_line(&space())), self.r_paren,
🤖 Fix all issues with AI agents
In `@crates/biome_js_formatter/src/js/assignments/array_assignment_pattern.rs`:
- Around line 26-33: The empty array pattern isn't honoring delimiterSpacing;
change the formatting in array_assignment_pattern.rs so the delimiter spacing
flag (should_insert_space_around_brackets) is applied even for empty patterns:
ensure the call to soft_block_indent_with_maybe_space(&elements.format(),
should_insert_space_around_brackets) runs when elements are empty (detect via
elements.is_empty() or equivalent) and emit the inner space when delimiter
spacing is true (so empty [] becomes [ ]); update the related test fixture to
expect the spaced empty pattern when delimiterSpacing is enabled.
In `@crates/biome_js_formatter/src/js/classes/setter_class_member.rs`:
- Around line 25-43: The space placement inside the should_insert_space branch
can produce double spaces when a trailing comma is present; adjust the emitter
so spacing is consistent by moving the space() that currently follows
comma_token.format() to instead precede r_paren_token.format(), or conditionally
emit that space only when comma_token is not a trailing-comma token. Locate the
block that builds the sequence (referencing modifiers.format(),
set_token.format(), name.format(), l_paren_token.format(), parameter.format(),
comma_token.format(), r_paren_token.format(), body.format()) and either move the
space() from after comma_token to immediately before r_paren_token.format() or
add a conditional around that space based on whether comma_token represents a
present trailing comma.
In `@crates/biome_js_formatter/src/js/expressions/arrow_function_expression.rs`:
- Around line 125-132: Inline sequence-expression parens are missing the
trailing space because soft_block_indent_with_maybe_space only emits the leading
space; update the inline sequence-expression code in
arrow_function_expression.rs to insert if_group_fits_on_line(&space())
immediately before the closing token(")") in both places where
soft_block_indent_with_maybe_space is used (the two inline sequence-expression
paths), so the formatter emits "(a, b )" spacing correctly when the group fits
on one line.
In `@crates/biome_js_formatter/src/js/expressions/unary_expression.rs`:
- Line 5: The unary-operator spacing logic in the JsUnaryExpression formatter is
inserting a space after the logical-not operator, turning "!foo" into "! foo";
update the formatting in the JsUnaryExpression handling so that when the
operator token is the logical-not ('!' or UnaryOp::LogicalNot) you do not emit
an extra space between the '!' operator and its operand — keep '!' glued to the
operand (and likewise preserve '!!a' by not splitting the two '!' tokens); leave
spacing decisions for parentheses to the operand's formatter and only apply
delimiter-interior spacing to other operators as originally intended.
In `@crates/biome_js_formatter/src/js/statements/while_statement.rs`:
- Around line 21-33: The closing parenthesis lacks a symmetric space when
delimiter_spacing is enabled, producing "while ( a)"; update the formatting
block that builds the while statement (around while_token.format(),
l_paren_token.format(),
group(&soft_block_indent_with_maybe_space(&test.format(), should_insert_space)),
r_paren_token.format()) to conditionally insert a space or soft break before
r_paren_token.format() when should_insert_space is true (i.e., use a
space/soft-line-break token analogous to the leading space so output becomes
"while ( a )"); modify the group composition to include that conditional spacer
rather than leaving the trailing side empty.
In `@crates/biome_js_formatter/src/jsx/auxiliary/spread_child.rs`:
- Around line 54-69: When comments are present and delimiter_spacing is true the
group created by group(&soft_block_indent_with_maybe_space(&format_inner, true))
only adds a leading space so the output becomes "{ …}" instead of "{ … }";
update the branch handling f.comments().has_comments(expression.syntax()) to
include a trailing soft space/line-break inside that group (i.e. ensure
soft_block_indent_with_maybe_space or the group call around format_inner appends
a trailing space/soft-break), referencing the existing symbols
delimiter_spacing, format_inner, soft_block_indent_with_maybe_space and group so
the single-line output includes both leading and closing spacing when comments
exist.
🧹 Nitpick comments (6)
crates/biome_js_formatter/tests/specs/js/module/delimiter-spacing/parenthesized.js (1)
31-35: Minor: verify character counts in boundary test comments.The comments claim specific character counts (78/80 for line 32, 79/81 for line 35), but a quick count suggests these may be off by 1-2 characters. Not a blocker since the test infrastructure will validate the actual formatter output, but accurate comments help future maintainers understand the test's intent.
crates/biome_js_formatter/src/js/classes/setter_class_member.rs (1)
25-61: Consider reducing duplication between the two branches.Both branches share the same formatting for
modifiers,set_token,name, andbody. Only the spacing around the parameter differs. A small refactor could reduce the duplication, though this is optional.♻️ Optional refactor to reduce duplication
let (pre_param_space, post_param_space) = if should_insert_space { (space(), space()) } else { (empty_element(), empty_element()) }; write![ f, [ modifiers.format(), space(), set_token.format(), space(), name.format(), l_paren_token.format(), pre_param_space, parameter.format(), comma_token.format(), post_param_space, r_paren_token.format(), space(), body.format(), ] ]crates/biome_css_formatter/src/tailwind/auxiliary/source_inline.rs (1)
18-40: Consider reducing duplication.The branching duplicates the formatting of
inline_token,l_paren_token,content, andr_paren_token. A minor refactor could extract common elements:♻️ Suggested refactor
- if should_insert_space { - write!( - f, - [ - inline_token.format(), - l_paren_token.format(), - space(), - &content.format(), - space(), - r_paren_token.format(), - ] - ) - } else { - write!( - f, - [ - inline_token.format(), - l_paren_token.format(), - &content.format(), - r_paren_token.format(), - ] - ) - } + write!(f, [inline_token.format(), l_paren_token.format()])?; + if should_insert_space { + write!(f, [space()])?; + } + write!(f, [&content.format()])?; + if should_insert_space { + write!(f, [space()])?; + } + write!(f, [r_paren_token.format()])Alternatively, if the content doesn't require soft-indent semantics, the current explicit approach is perfectly readable—just a nitpick.
crates/biome_css_formatter/src/css/auxiliary/document_custom_matcher.rs (1)
22-44: Consider reducing duplication with a helper or conditional formatting.The two branches are nearly identical, differing only by the
space()calls. This is functional but could be streamlined. That said, the current approach is clear and readable.♻️ Optional: Reduce duplication
- if should_insert_space { - write!( - f, - [ - name.format(), - l_paren_token.format(), - space(), - value.format(), - space(), - r_paren_token.format() - ] - ) - } else { - write!( - f, - [ - name.format(), - l_paren_token.format(), - value.format(), - r_paren_token.format() - ] - ) - } + write!(f, [name.format(), l_paren_token.format()])?; + if should_insert_space { + write!(f, [space()])?; + } + write!(f, [value.format()])?; + if should_insert_space { + write!(f, [space()])?; + } + write!(f, [r_paren_token.format()])crates/biome_js_formatter/src/ts/auxiliary/index_signature_type_member.rs (1)
31-45: Consider usingsoft_block_indent_with_maybe_spacefor consistency.The current implementation adds
space()around thesoft_block_indent, which may produce unexpected output when the content breaks across lines (e.g.,[ \n param\n ]). Other formatters in this PR usesoft_block_indent_with_maybe_spacewhich handles this more elegantly.Suggested refactor
- if delimiter_spacing { - write![ - f, - [ - group(&format_args![ - l_brack_token.format(), - space(), - soft_block_indent(&format_args![parameter.format()]), - space(), - r_brack_token.format(), - ]), - type_annotation.format(), - FormatTypeMemberSeparator::new(separator_token.as_ref()), - ] - ] - } else { - write![ - f, - [ - group(&format_args![ - l_brack_token.format(), - soft_block_indent(&format_args![parameter.format()]), - r_brack_token.format(), - ]), - type_annotation.format(), - FormatTypeMemberSeparator::new(separator_token.as_ref()), - ] - ] - } + write![ + f, + [ + group(&format_args![ + l_brack_token.format(), + soft_block_indent_with_maybe_space(¶meter.format(), delimiter_spacing), + r_brack_token.format(), + ]), + type_annotation.format(), + FormatTypeMemberSeparator::new(separator_token.as_ref()), + ] + ]crates/biome_js_formatter/src/ts/types/indexed_access_type.rs (1)
35-44: Minor: consider grouping for consistent breaking behaviour.The non-spacing branch doesn't use
group(), whilst the spacing branch does. This could lead to subtle differences in line-breaking decisions between the two modes. Worth double-checking this is intentional via your test snapshots.
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@crates/biome_js_formatter/tests/specs/js/module/delimiter-spacing/setters.js`:
- Around line 129-137: The boundary test comments for the setter lines are
incorrect: in class Foo19 (set a with ...uv) update the comment "78+2=80 chars
(fits)" to the correct "76+2=78 chars (fits)"; in class Foo20 (set a with
...uvw) update the comment "79+2=81 chars (breaks)" to "77+2=79 chars (breaks)
so the annotated character counts match the actual line lengths used by the
tests.
|
It would be good to also update some of the rule metadata in |
|
Sure, I'll try to address that as well. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_cli/src/execute/migrate/unsupported_rules.rs (1)
245-246:⚠️ Potential issue | 🟠 MajorUpdate
type-generic-spacingto map toFormatterOption("delimiterSpacing").The
delimiterSpacingoption for JavaScript explicitly covers TypeScript angle brackets (<>), which is precisely whattype-generic-spacingenforces. This mapping aligns with other delimiter-related rules likearray-bracket-spacing,space-in-brackets, andtemplate-curly-spacing, which already map todelimiterSpacing. Change fromFormatterCoverstoFormatterOption("delimiterSpacing")on line 245.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_cli/src/execute/migrate/unsupported_rules.rs` around lines 245 - 246, Change the mapping for the ESLint stylistic rule named by EslintStylistic("type-generic-spacing") so it uses a formatter option instead of a blanket cover; replace the UnsupportedRule entry that currently pairs EslintStylistic("type-generic-spacing") with FormatterCovers and map it to FormatterOption("delimiterSpacing") (i.e., update the UnsupportedRule for EslintStylistic("type-generic-spacing") to use FormatterOption("delimiterSpacing") so it aligns with other delimiter rules).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/biome_cli/src/execute/migrate/unsupported_rules.rs`:
- Around line 245-246: Change the mapping for the ESLint stylistic rule named by
EslintStylistic("type-generic-spacing") so it uses a formatter option instead of
a blanket cover; replace the UnsupportedRule entry that currently pairs
EslintStylistic("type-generic-spacing") with FormatterCovers and map it to
FormatterOption("delimiterSpacing") (i.e., update the UnsupportedRule for
EslintStylistic("type-generic-spacing") to use
FormatterOption("delimiterSpacing") so it aligns with other delimiter rules).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58946b7c-1bcc-402f-ab44-f8c42dc10199
📒 Files selected for processing (1)
crates/biome_cli/src/execute/migrate/unsupported_rules.rs
@coderabbitai, |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/unsupported_rules.rs`:
- Around line 108-111: Remove the UnsupportedRule entries that map
Eslint("template-curly-spacing") to FormatterOption("delimiterSpacing");
specifically locate the UnsupportedRule(...) instances containing
Eslint("template-curly-spacing") and FormatterOption("delimiterSpacing") and
delete them so the migrator no longer directs users to the incorrect
delimiterSpacing option. Ensure only those three mappings (the ones pairing
template-curly-spacing with delimiterSpacing) are removed and leave other
UnsupportedRule entries untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b1cdde95-7c06-4d72-94ed-b978f80a0ade
📒 Files selected for processing (1)
crates/biome_cli/src/execute/migrate/unsupported_rules.rs
|
Thank you for this! So, this is a massive PR and I wonder why this wasn't split in the first place. It will take us a while to review it, and since we're also working on SCSS, it will take you a lot of work to keep the PR up to date. Wouldn't be better to implement this by language? As for docs, I gave it a glance, I already started seeing a problem: they are very generic and repeated. We should aim for preciseness, for users and us. I want to understand the criteria of option, so that if I need to triage a bug, I know what to do. For example |
|
Thanks for taking a look, @ematipico! I really appreciate it.
I've improved the docs in this commit, but if you still think they are not accurate enough, please give me an example of how you think they should be and I'll try to improve them. Once we have them right, I'll also update the PR on the website.
I can split the PR into three or add the option to the SCSS part and keep it up to date, whatever you folks prefer. |
It would be great if you could split up the PR in multiple PRs (as you see fit). That would help. ,For example I saw a bug but I lost the file because there are too many. I recall that in |
I've split the PR into all these branches, but I've just realized that they can't be opened with the correct base if some branches originate from others when all of them are coming from a fork. What do you want me to do? Should I open them all against
All branches target |
Yeah, that sounds good! HEADS UP! I just merged a few things around the formatter testing infrastructure. It's now less noisy, and the snapshots are different. You should see different outputs from the snapshots, and less changes. Thankfully, the PR should be more bearable. I'll help you to bring the PRs to the finish line |
Looks great! However, I see some errors in Do you want me to open a PR to fix those things before opening mines, or could you let me know when that's resolved? Thank you very much. |
|
I can fix those, so you can focus on your stuff. If they prevent you from moving forward, feel free to fix them on your branch (the formatting one). I'll help to update the PRs once the fixes are in next |
|
Great, thank you very much. Let me know when |
|
@luisherranz the next branch should be good to go |
|
Thanks @ematipico! I've already opened all the PRs against
|
|
Superseed by #9718. |
|
The main branch adding the option and all the JavaScript branches are already ready for review. Tomorrow or the day after, I'll continue with CSS, JSON, and ESLint. |
Huge kudos to @orballo for doing most of the work in discovering which parts of the Biome formatter needed to be changed to add this option in his proof of concept: orballo#1
Summary
This PR adds a new
delimiterSpacingformatter option that inserts spaces inside delimiters (parentheses, square brackets, TS angle brackets, and JSX curly braces).Closes #4607
When enabled, this input:
Is formatted as:
The option is available for:
(), square brackets[], TS angle brackets<>, and JSX curly braces{}[]()and square brackets[]Configuration example:
{ "formatter": { "delimiterSpacing": true } }Or per-language:
{ "javascript": { "formatter": { "delimiterSpacing": true } } }See #2360 (comment).
AI usage
As the Gutenberg project is using a non-official fork of Prettier, I run @orballo's Biome version in the Gutenberg repository and used AI to extract all the tests and edge cases. Then the AI assisted me in converting the changes made by @orballo's proof of concept to changes in the current formatter.
Test Plan
Website
Website docs and playground integration PR in this website repo PR: