+
Skip to content

Conversation

bavalpey
Copy link
Contributor

@bavalpey bavalpey commented May 28, 2025

Summary

Fixes #5879

Adds a new option to the javascript formatter, operator_linebreak that configures whether linebreaks for binary expressions should occur before or after the binary operator.

Prettier has implemented this: prettier/prettier#7111

Test Plan

I wrote tests for this that are on par with the other tests for formatting options.
It tests that when the option is enabled, long lines break before the binary operator instead of after.
I based it off of similar formatting options tests, each of which had only one unit test and weren't really testing edge cases. I presume that this is sufficient.

Additional Information

There are nearly 0 logic changes required for this.

To see how it works:
For the javascript file

const VERY_LONG_CONDITION_1234123412341234123412341234 = false;

if (VERY_LONG_CONDITION_1234123412341234123412341234 && VERY_LONG_CONDITION_1234123412341234123412341234 && VERY_LONG_CONDITION_1234123412341234123412341234 && VERY_LONG_CONDITION_1234123412341234123412341234) {
  console.log("DONE")
}

running with the option --javascript-formatter-operator-linebreak=before produces

  ✖ Formatter would have printed the following content:
  
    1  1 │   const VERY_LONG_CONDITION_1234123412341234123412341234 = false;
    2  2 │   
    3    │ - if·(VERY_LONG_CONDITION_1234123412341234123412341234·&&·VERY_LONG_CONDITION_1234123412341234123412341234·&&·VERY_LONG_CONDITION_1234123412341234123412341234·&&·VERY_LONG_CONDITION_1234123412341234123412341234)·{
    4    │ - ··console.log("DONE")
       3 │ + if·(
       4 │ + → VERY_LONG_CONDITION_1234123412341234123412341234
       5 │ + → &&·VERY_LONG_CONDITION_1234123412341234123412341234
       6 │ + → &&·VERY_LONG_CONDITION_1234123412341234123412341234
       7 │ + → &&·VERY_LONG_CONDITION_1234123412341234123412341234
       8 │ + )·{
       9 │ + → console.log("DONE");
    5 10 │   }
    6    │ - 
    7 11 │   

@github-actions github-actions bot added A-CLI Area: CLI A-Project Area: project A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels May 28, 2025
@bavalpey bavalpey changed the title feat feat(formatter): Add option to split binary expressions before operators May 28, 2025
@bavalpey bavalpey changed the title feat(formatter): Add option to split binary expressions before operators feat(formatter): add option to split binary expressions before operators May 28, 2025
Copy link

codspeed-hq bot commented May 29, 2025

CodSpeed Performance Report

Merging #6159 will degrade performances by 61.19%

Comparing bavalpey:break-before-binary-ops (0dfeee9) with next (275cc8b)1

🎉 Hooray! codspeed-rust just leveled up to 3.0.3!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

⚡ 1 improvements
❌ 3 regressions
✅ 110 untouched benchmarks
🆕 1 new benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
js_analyzer[index_3894593175024091846.js] 49.4 ms 53 ms -6.75%
js_analyzer[parser_13571644119461115204.ts] 79.4 ms 85.3 ms -6.94%
🆕 module_graph[@next/font/google/index.d.ts] N/A 290.2 ms N/A
deserialize_from_json_str[package.json] 1,038.3 µs 930.8 µs +11.55%
deserialize_from_json_str[tsconfig.json] 218 µs 561.8 µs -61.19%

Footnotes

  1. No successful run was found on next (90c5d6b) during the generation of this report, so f00af73 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@bavalpey
Copy link
Contributor Author

bavalpey commented May 30, 2025

I'm not understanding some of the checks that are failing, like the codegen and documentation.

Is there a step I am supposed to do to fix the codegen? (test output: https://github.com/biomejs/biome/actions/runs/15312085380/job/43092346301?pr=6159)

@ematipico
Copy link
Member

ematipico commented May 30, 2025

I'm not understanding some of the checks that are failing, like the codegen and documentation.

Is there a step I am supposed to do to fix the codegen? (test output: biomejs/biome/actions/runs/15312085380/job/43092346301?pr=6159)

We have a command that takes care of the codegen: just gen-all. For this particular case, you can run just gen-bindings. Since you updated the configuration, the schema must be updated.

Here the section of the contribution guide: https://github.com/biomejs/biome/blob/main/CONTRIBUTING.md#checks

@bavalpey
Copy link
Contributor Author

bavalpey commented Jun 7, 2025

I'm not understanding some of the checks that are failing, like the codegen and documentation.
Is there a step I am supposed to do to fix the codegen? (test output: biomejs/biome/actions/runs/15312085380/job/43092346301?pr=6159)

We have a command that takes care of the codegen: just gen-all. For this particular case, you can run just gen-bindings. Since you updated the configuration, the schema must be updated.

Here the section of the contribution guide: https://github.com/biomejs/biome/blob/main/CONTRIBUTING.md#checks

Thanks. I've followed the steps and run this now.

I wasn't sure if I needed to add a changelog for this, as the instructions for changelogs seemed to be directed towards PRs that were adding a new minor release? Surely you would bundle something like this with other changes for a minor release?

Let me know if there's anything additional I need to do to get this in. I'm very keen on having this option in.

@bavalpey
Copy link
Contributor Author

bavalpey commented Jun 8, 2025

Hmm.
I've no idea why benchmarks are now timing out? All I did was run the command to update the schema.

It worked before I did this: https://github.com/biomejs/biome/actions/runs/15348620153

Now it's failing, saying the action timed out: https://github.com/biomejs/biome/actions/runs/15512128393/job/43686071399?pr=6159

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @bavalpey

The PR looks good. I am going to block it because we're close to ship v2 and we don't want to ship more features than we already have. We will merge it and ship in v1

The failures in CI aren't a concern, don't worry. You did splendid.

@PointSingularity
Copy link
Contributor

Is this considered for the next release?

@ematipico
Copy link
Member

Is this considered for the next release?

Yes, for the next minor. No ETA

@arendjr arendjr added this to the Biome 2.1 milestone Jun 23, 2025
@PointSingularity
Copy link
Contributor

Hoping this one comes in the next patch 🤞
(need to migrate the whole codebase from prettier, but don't want to submit a 500 file change PR twice)

@ematipico
Copy link
Member

ematipico commented Jul 8, 2025

Oh, damn, that's on us 😢 apologies! We should have added it to next minor, and forgot about it. Let me create a milestone now so I won't forget.

In the meantime, @bavalpey, can you please point this PR to the next branch?

@ematipico ematipico modified the milestones: Biome 2.1, Biome v2.2 Jul 8, 2025
@bavalpey bavalpey changed the base branch from main to next July 8, 2025 18:08
@bavalpey
Copy link
Contributor Author

bavalpey commented Jul 8, 2025

In the meantime, @bavalpey, can you please point this PR to the next branch?

Done. But there's now conflicts. What's the protocol for resolving these? Does a maintainer generally edit?

@ematipico
Copy link
Member

If you're in a position to resolve the conflicts, that would be great. Otherwise we do it when we can

@ematipico
Copy link
Member

Hey @bavalpey , will you be able to resolve the conflicts?

@bavalpey
Copy link
Contributor Author

Hey @bavalpey , will you be able to resolve the conflicts?

Sure let me take a look. Not sure how involved they are.

Copy link

changeset-bot bot commented Jul 22, 2025

🦋 Changeset detected

Latest commit: 0dfeee9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
Name Type
@biomejs/biome Minor
@biomejs/cli-win32-x64 Minor
@biomejs/cli-win32-arm64 Minor
@biomejs/cli-darwin-x64 Minor
@biomejs/cli-darwin-arm64 Minor
@biomejs/cli-linux-x64 Minor
@biomejs/cli-linux-arm64 Minor
@biomejs/cli-linux-x64-musl Minor
@biomejs/cli-linux-arm64-musl Minor
@biomejs/wasm-web Minor
@biomejs/wasm-bundler Minor
@biomejs/wasm-nodejs Minor
@biomejs/backend-jsonrpc Patch
@biomejs/js-api Major

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

@bavalpey
Copy link
Contributor Author

Hey @bavalpey , will you be able to resolve the conflicts?

Done

@bavalpey bavalpey requested a review from ematipico July 22, 2025 23:01
@ematipico
Copy link
Member

Thank you. Can you please add a changeset too? That's missing and we require it

@bavalpey
Copy link
Contributor Author

Thank you. Can you please add a changeset too? That's missing and we require it

Done. Let me know if it looks sufficient.

@bavalpey
Copy link
Contributor Author

bavalpey commented Jul 23, 2025

Hmm.. Not sure why the PR I did just to fix the merge conflict would cause an unrelated test to break...
Or a performance regression, for that matter.

@siketyan
Copy link
Member

@bavalpey You can just update the snapshots to make the failing tests pass. You can ignore the performance regression too

@bavalpey
Copy link
Contributor Author

@bavalpey You can just update the snapshots to make the failing tests pass. You can ignore the performance regression too

Oh, right. New snapshots would've been added since merge, and I forgot to update them. Thanks!

@ematipico ematipico merged commit f02a296 into biomejs:next Jul 26, 2025
27 of 28 checks passed
@7Hua
Copy link

7Hua commented Jul 31, 2025

"javascript": { "formatter": { "indentStyle": "space", "indentWidth": 4, "bracketSpacing": false, "quoteStyle": "single", "trailingCommas": "none", "arrowParentheses": "asNeeded", "operator-linebreak": "before", } },。Why am I still unable to use operator-linebreak in biome, and it prompts "Found an unknown key operator-linebreak"?

@ematipico
Copy link
Member

ematipico commented Jul 31, 2025

Because it hasn't been released yet. It will be available in the next minor

ematipico added a commit that referenced this pull request Aug 12, 2025
…ors (#6159)

Co-authored-by: bavalpey <bavalpey@users.noreply.github.com>
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

Co-authored-by: ematipico <602478+ematipico@users.noreply.github.com>
Co-authored-by: PointSingularity <56246797+PointSingularity@users.noreply.github.com>
Co-authored-by: siketyan <12772118+siketyan@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-CLI Area: CLI A-Formatter Area: formatter A-Project Area: project L-JavaScript Language: JavaScript and super languages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

📎 Break lines before binary operators

6 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载