-
-
Notifications
You must be signed in to change notification settings - Fork 724
feat(formatter): add option to split binary expressions before operators #6159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CodSpeed Performance ReportMerging #6159 will degrade performances by 61.19%Comparing 🎉 Hooray!
|
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
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) |
We have a command that takes care of the codegen: 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. |
Hmm. 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 |
There was a problem hiding this 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.
Is this considered for the next release? |
Yes, for the next minor. No ETA |
Hoping this one comes in the next patch 🤞 |
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 |
Done. But there's now conflicts. What's the protocol for resolving these? Does a maintainer generally edit? |
If you're in a position to resolve the conflicts, that would be great. Otherwise we do it when we can |
Hey @bavalpey , will you be able to resolve the conflicts? |
Sure let me take a look. Not sure how involved they are. |
🦋 Changeset detectedLatest commit: 0dfeee9 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 |
Done |
Thank you. Can you please add a changeset too? That's missing and we require it |
Done. Let me know if it looks sufficient. |
Hmm.. Not sure why the PR I did just to fix the merge conflict would cause an unrelated test to break... |
@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! |
|
Because it hasn't been released yet. It will be available in the next minor |
…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>
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
running with the option
--javascript-formatter-operator-linebreak=before
produces