challenge(formatter): add `bracketSpacing` option matching Prettier's setting #627

faultyserver · 2023-10-30T00:23:29Z

Summary

This PR adds a new bracketSpacing option for the js formatter which matches Prettier's equivalent option. This option controls whether spaces are inserted around "object-like literals", meaning object expressions, named import/export specifier lists, import assertions, and type expressions.

The following is a case that should cover pretty much every variation that this option affects:

import {a, b, c} from "foo" assert {type: "json"};

type X = {a: string};
type Y = {
  a: string;
};

const {a, b} = {a, b};
let {foo, bar, baz, zzz, andareallylongname} = {
  foo,
  bar,
  baz,
  zzz,
  andareallylongname,
};

export {a, b as car};

type Foo<A extends {type: string}> = A<{type: B}>;

With bracketSpacing: false, this code should appear unchanged after formatting.

By default, bracketSpacing is left as true, just like in Prettier, and users will have to explicitly disable it to see any changes from their current formatting.

The goal of adding this option is to ease adoption from existing Prettier users. While Biome does not (and I'm assuming won't) aim to have perfect parity with Prettier's features, bracket spacing is a commonly-set option, and one that causes a massive diff for large codebases.

Not much other discussion has happened on this option, other than this post from a little over a month ago: #392.

Implementation Notes

I'm both relatively new to Rust and completely new to this project, so I looked for inspiration on how to add an option by finding a previous addition, arrowParentheses, and copying over the plumbing, then implementing the feature, and finally adding the relevant tests.

I decided to use a plain bool value for the option rather than a string/enum only because that's what Prettier does. If a string value would be preferred, I'm happy to change it to match those as well.

Test Plan

cargo test with a bunch of new snapshots. A local playground link with the sample code above also demonstrates the behavior.

faultyserver · 2023-10-30T00:32:20Z

crates/biome_formatter/src/builders.rs

+pub fn conditional_space(should_insert: bool) -> Option<Space> {
+    if should_insert {
+        Some(Space)
+    } else {
+        None
+    }
+}
+


I added this conditional_space function just for clarity at the callsites. Not sure if it has too much utility outside of this option, but I like the "branchless" flow from the caller side while still making it clear that the space may or may not exist.

Instead of using conditional_space(cond) at call site, did you try cond.then_some(space())?

Instead of using conditional_space(cond) at call site, did you try cond.then_some(space())?

That wouldn't be very ergonomic when writing the IR, and it saves us a bunch of branching.

I can totally see the usage of this builder because our infra can understand Option. I would call this function maybe_space (sorry for the bike-shedding) because it's a prefix we already used through the code base,.

I can definitely agree with maybe_*. Honestly idk why I didn't go with that in the first place since it's normally what I end up using lol.

I didn't actually know .then_some existed when I wrote this code (still very new to Rust), and had I known I might've used that instead. But I do like the declarative nature of saying maybe_ for things, along with the advantage of working with any boolean value, not just an Option.

PR should be updated with the name change now.

crates/biome_formatter/src/builders.rs

crates/biome_js_formatter/src/js/module/export_named_clause.rs

crates/biome_js_formatter/src/js/module/export_named_from_clause.rs

crates/biome_js_formatter/src/ts/auxiliary/property_signature_type_member.rs

Conaclos · 2023-11-15T12:29:52Z

@faultyserver you can be interested by #720

faultyserver · 2023-11-15T20:51:29Z

Happy to turn this into a challenge feature! I hadn't seen anything about whether there was a goal to support all of Prettier's options as part of the challenge or not.

SuperchupuDev · 2023-11-18T17:31:40Z

@faultyserver FYI we just got confirmation that the challenge does aim for all prettier js options, after we asked for clarification

faultyserver · 2023-11-18T19:27:30Z

I ended up basically re-implementing the feature after trying to rebase off main and somehow ended up with 20 fewer files changed lol.

But I think this is now ready to go again pending CI.

ematipico

The implementation looks good, and fortunately, this option doesn't add much friction in terms of maintenance.

I added some comments we should address before merging, although I am very pleased to see this contribution, thank you @faultyserver !

ematipico · 2023-11-19T15:53:53Z

crates/biome_service/src/configuration/javascript/formatter.rs

@@ -36,6 +36,10 @@ pub struct JavascriptFormatter {
    #[bpaf(long("arrow-parentheses"), argument("always|as-needed"), optional)]
    #[serde(skip_serializing_if = "Option::is_none")]
    pub arrow_parentheses: Option<ArrowParentheses>,
+    /// Whether to insert spaces around brackets in object literals. Defaults to true.
+    #[bpaf(long("bracket-spacing"), argument("BOOLEAN"), optional)]


For arguments that are true/false, at the moment we use different implementation, e.g.

#[bpaf(long("vcs-use-ignore-file"), argument("true|false"), optional)] pub use_ignore_file: Option<bool>,

We could change all of them to use BOOLEAN, but for now we should be more consistent. .

ematipico · 2023-11-19T16:04:52Z

crates/biome_formatter/src/builders.rs

+pub fn conditional_space(should_insert: bool) -> Option<Space> {
+    if should_insert {
+        Some(Space)
+    } else {
+        None
+    }
+}
+


Instead of using conditional_space(cond) at call site, did you try cond.then_some(space())?

That wouldn't be very ergonomic when writing the IR, and it saves us a bunch of branching.

I can totally see the usage of this builder because our infra can understand Option. I would call this function maybe_space (sorry for the bike-shedding) because it's a prefix we already used through the code base,.

crates/biome_js_formatter/src/js/module/export_named_from_clause.rs

ematipico · 2023-11-19T16:07:55Z

crates/biome_formatter/src/builders.rs

+/// # Ok(())
+/// # }
+/// ```
+pub fn soft_block_indent_with_conditional_space<Context>(


Same as the previous suggestion: soft_block_indent_with_maybe_space

SuperchupuDev

you forgot one minor detail, but other than that, looks good!

CHANGELOG.md

ematipico

Great! We just need to fix the conflicts with the new options we just added and we can merge the PR! Again, great work here

SuperchupuDev

i can't really properly review a PR like this without my browser tab lagging but i'm approving to remove the previous requested changes review. great work! thanks a lot for contributing ❤

faultyserver · 2023-11-19T22:39:32Z

should hopefully be updated with no conflitcs now 🤞

faultyserver · 2023-11-19T23:29:59Z

i should stop assuming before CI passes, but now finally it's all set.

github-actions bot added A-CLI Area: CLI A-Project Area: project A-Formatter Area: formatter A-Website Area: website L-JavaScript Language: JavaScript and super languages A-Changelog Area: changelog labels Oct 30, 2023

faultyserver commented Oct 30, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

faultyserver changed the title ~~feat(formatter): add bracketSpacing option matching Prettier's setting~~ challenge(formatter): add bracketSpacing option matching Prettier's setting Nov 15, 2023

SuperchupuDev mentioned this pull request Nov 18, 2023

☂️ Prettier code challenge hub #720

Closed

faultyserver force-pushed the feature/bracket-spacing branch from 727af4d to 9375775 Compare November 18, 2023 18:00

github-actions bot added A-Linter Area: linter A-Tooling Area: internal tools labels Nov 18, 2023

faultyserver force-pushed the feature/bracket-spacing branch from 9375775 to 01d6202 Compare November 18, 2023 19:26

github-actions bot removed A-Linter Area: linter A-Tooling Area: internal tools labels Nov 18, 2023

ematipico approved these changes Nov 19, 2023

View reviewed changes

faultyserver force-pushed the feature/bracket-spacing branch from ffa37b8 to 13f74f3 Compare November 19, 2023 17:56

SuperchupuDev requested changes Nov 19, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

ematipico approved these changes Nov 19, 2023

View reviewed changes

SuperchupuDev approved these changes Nov 19, 2023

View reviewed changes

faultyserver added 4 commits November 19, 2023 22:53

add rust plumbing for bracket-spacing option

c01394e

update test snapshots

3b4c297

implement feature and add tests

4e02c32

js packages/changelog/etc

d301911

faultyserver added 4 commits November 19, 2023 22:53

fix js formatting

6cca2d7

pr feedback

fdccda6

fix changelog

ce3cc66

update snapshots

008d490

faultyserver force-pushed the feature/bracket-spacing branch from 3471369 to 008d490 Compare November 19, 2023 22:56

codegen website

9d81d00

Conaclos merged commit e60c49b into biomejs:main Nov 19, 2023
14 checks passed

faultyserver deleted the feature/bracket-spacing branch November 19, 2023 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

challenge(formatter): add `bracketSpacing` option matching Prettier's setting #627

challenge(formatter): add `bracketSpacing` option matching Prettier's setting #627

faultyserver commented Oct 30, 2023

faultyserver Oct 30, 2023

Conaclos Nov 19, 2023

ematipico Nov 19, 2023

faultyserver Nov 19, 2023

This comment has been minimized.

Conaclos commented Nov 15, 2023

faultyserver commented Nov 15, 2023

SuperchupuDev commented Nov 18, 2023

faultyserver commented Nov 18, 2023

ematipico left a comment

ematipico Nov 19, 2023

ematipico Nov 19, 2023

ematipico Nov 19, 2023

SuperchupuDev left a comment

ematipico left a comment

SuperchupuDev left a comment

faultyserver commented Nov 19, 2023 •

edited

faultyserver commented Nov 19, 2023

challenge(formatter): add bracketSpacing option matching Prettier's setting #627

challenge(formatter): add bracketSpacing option matching Prettier's setting #627

Conversation

faultyserver commented Oct 30, 2023

Summary

Implementation Notes

Test Plan

faultyserver Oct 30, 2023

Choose a reason for hiding this comment

Conaclos Nov 19, 2023

Choose a reason for hiding this comment

ematipico Nov 19, 2023

Choose a reason for hiding this comment

faultyserver Nov 19, 2023

Choose a reason for hiding this comment

This comment has been minimized.

Conaclos commented Nov 15, 2023

faultyserver commented Nov 15, 2023

SuperchupuDev commented Nov 18, 2023

faultyserver commented Nov 18, 2023

ematipico left a comment

Choose a reason for hiding this comment

ematipico Nov 19, 2023

Choose a reason for hiding this comment

ematipico Nov 19, 2023

Choose a reason for hiding this comment

ematipico Nov 19, 2023

Choose a reason for hiding this comment

SuperchupuDev left a comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

SuperchupuDev left a comment

Choose a reason for hiding this comment

faultyserver commented Nov 19, 2023 • edited

faultyserver commented Nov 19, 2023

challenge(formatter): add `bracketSpacing` option matching Prettier's setting #627

challenge(formatter): add `bracketSpacing` option matching Prettier's setting #627

faultyserver commented Nov 19, 2023 •

edited