feat(formatter): add delimiterSpacing option#8966
feat(formatter): add delimiterSpacing option#8966luisherranz wants to merge 9 commits intobiomejs:nextfrom
delimiterSpacing option#8966Conversation
chore(formatter): add changeset
🦋 Changeset detectedLatest commit: a91e20b 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 |
WalkthroughAdds a new formatter option Possibly related PRs
Suggested labels
🚥 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)
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 |
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.
crates/biome_js_formatter/src/js/assignments/array_assignment_pattern.rs
Show resolved
Hide resolved
crates/biome_js_formatter/src/js/expressions/arrow_function_expression.rs
Show resolved
Hide resolved
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.
crates/biome_js_formatter/tests/specs/js/module/delimiter-spacing/setters.js
Show resolved
Hide resolved
|
It would be good to also update some of the rule metadata in |
|
Sure, I'll try to address that as well. |
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: