Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_formatter): Binary expression formatting #3079

Merged
merged 3 commits into from Aug 19, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Aug 17, 2022

Summary

Part of #3046

This PR refactors our JsAnyBinaryLikeExpression formatting to closer match prettier's formatting. The main change is that our old implementation used to group the operator with the left hand side:

a && b && c

became

group("a &&", [group("b &&", c)])

whereas prettier (and this PR) always groups the operator with the right-hand side

"a", group("&& b", group(" && c"))

This PR further enables the NeedsParentheses implementation for binary expressions.

Unsupported case

The new implementation doesn't support the issue-7024 test case

const radioSelectedAttr =
  (isAnyValueSelected &&
    node.getAttribute(radioAttr.toLowerCase()) === radioValue) ||
  ((!isAnyValueSelected && values[a].default === true) || a === 0);

The issue is that removing the parentheses from ((!isAnyValueSelected && values[a].default === true) || a === 0) rebalances the tree. Prettier solves this problem by rebalancing logical expressions ahead of formatting. This is something Rome doesn't support today which is why I disabled parentheses removal from logical expression if the child and parent have the same operator but the child is the right side expression.

Test Plan

I manually reviewed the snapshot changes.

Average compatibility: 81.64 -> 82.59
Compatible lines: 78.42 -> 79.70

@MichaReiser MichaReiser added this to the 0.9.0 milestone Aug 17, 2022
@MichaReiser MichaReiser added the A-Formatter Area: formatter label Aug 17, 2022
@MichaReiser MichaReiser marked this pull request as ready for review August 17, 2022 16:15
@MichaReiser MichaReiser mentioned this pull request Aug 17, 2022
17 tasks
@MichaReiser MichaReiser requested a review from a team August 17, 2022 16:27
@cloudflare-pages
Copy link

cloudflare-pages bot commented Aug 17, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: db28af0
Status: ✅  Deploy successful!
Preview URL: https://dc29ad6a.tools-8rn.pages.dev
Branch Preview URL: https://feat-binary-expressions.tools-8rn.pages.dev

View logs

Base automatically changed from feat/needs-parentheses to main August 18, 2022 07:30
@MichaReiser MichaReiser linked an issue Aug 18, 2022 that may be closed by this pull request
@MichaReiser MichaReiser temporarily deployed to aws August 18, 2022 07:34 Inactive
@ematipico
Copy link
Contributor

What does "re-balance a tree" mean?

@github-actions
Copy link

github-actions bot commented Aug 18, 2022

@MichaReiser
Copy link
Contributor Author

MichaReiser commented Aug 18, 2022

What does "re-balance a tree" mean?

To change the tree's shape without changing the semantic meaning. In this particular case, it means that a tree that has nested logical expressions in the right side to the left side so that the right side.

  &&
a    &&
   b    c 

// becomes

    && 
 &&    c
a    b

The transformed version works well with our flattening logic where it will miss the former completely.

https://github.com/prettier/prettier/blob/d2bcaea121d244a00100e5c31e2e8ca9fe5f1ced/src/language-js/parse/postprocess/index.js#L207-L225

@MichaReiser MichaReiser temporarily deployed to aws August 18, 2022 07:55 Inactive
@MichaReiser MichaReiser temporarily deployed to aws August 18, 2022 12:20 Inactive
@MichaReiser MichaReiser merged commit 3b94a15 into main Aug 19, 2022
@MichaReiser MichaReiser deleted the feat/binary-expressions branch August 19, 2022 06:02
IWANABETHATGUY pushed a commit to IWANABETHATGUY/tools that referenced this pull request Aug 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parenthesized binary expression
2 participants