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

fix(rome_js_formatter): correctly format sequence expressions #2541

Merged
merged 5 commits into from
May 23, 2022

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented May 5, 2022

Summary

This PR fixes #2413

Test Plan

Added three cases that aim to fix them

Metrics

PR
File Based Average Prettier Similarity: 70.87%
Line Based Average Prettier Similarity: 64.93%

main

File Based Average Prettier Similarity: 70.84%
Line Based Average Prettier Similarity: 64.85%

@github-actions
Copy link

github-actions bot commented May 5, 2022

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45878 45878 0
Passed 44938 44938 0
Failed 940 940 0
Panics 0 0 0
Coverage 97.95% 97.95% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 39 39 0
Passed 36 36 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.31% 92.31% 0.00%

ts/babel

Test result main count This PR count Difference
Total 588 588 0
Passed 519 519 0
Failed 69 69 0
Panics 0 0 0
Coverage 88.27% 88.27% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 16257 16257 0
Passed 12391 12391 0
Failed 3866 3866 0
Panics 0 0 0
Coverage 76.22% 76.22% 0.00%

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 5, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 74888c2
Status: ✅  Deploy successful!
Preview URL: https://4ccbed25.tools-8rn.pages.dev

View logs

@github-actions
Copy link

github-actions bot commented May 5, 2022

@ematipico ematipico force-pushed the fix/sequence-expressions-indentation branch from 8b43836 to 093fe06 Compare May 23, 2022 10:05
@ematipico ematipico temporarily deployed to aws May 23, 2022 10:05 Inactive
@ematipico
Copy link
Contributor Author

PR rebased.

@ematipico ematipico temporarily deployed to aws May 23, 2022 13:10 Inactive
@ematipico ematipico requested a review from leops May 23, 2022 13:11
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

The IR of Rome now seems to differ from Prettier.
Prettier

[
  group([
    "____________first",
    ",",
    indent([line, "____________second"]),
    ",",
    indent([line, "____________third"]),
    ",",
    indent([line, "____________third"]),
    ",",
    indent([line, "____________third"]),
    ",",
    indent([line, "____________third"]),
    ",",
    indent([line, "____________third"]),
  ]),
  ";",
  hardline,
]

Rome

List [
    Group(
        List [
            SyntaxTokenSlice("____________first"),
            SyntaxTokenSlice(","),
            Line(Soft),
            Indent(
                List [
                    Line(SoftOrSpace),
                    SyntaxTokenSlice("____________second"),
                    SyntaxTokenSlice(","),
                ],
            ),
            Line(Soft),
            Indent(
                List [
                    Line(SoftOrSpace),
                    SyntaxTokenSlice("____________third"),
                    SyntaxTokenSlice(","),
                ],
            ),
            Line(Soft),
            Indent(
                List [
                    Line(SoftOrSpace),
                    SyntaxTokenSlice("____________third"),
                    SyntaxTokenSlice(","),
                ],
            ),
            Line(Soft),
            Indent(
                List [
                    Line(SoftOrSpace),
                    SyntaxTokenSlice("____________third"),
                    SyntaxTokenSlice(","),
                ],
            ),
            Line(Soft),
            Indent(
                List [
                    Line(SoftOrSpace),
                    SyntaxTokenSlice("____________third"),
                    SyntaxTokenSlice(","),
                ],
            ),
            Line(Soft),
            Indent(
                List [
                    Line(SoftOrSpace),
                    SyntaxTokenSlice("____________third"),
                ],
            ),
        ],
    ),
    StaticToken(";"),
    Line(Hard),
]

Why does Rome need the Line(soft). Seems to be redundant to the SoftOrSpace.

@ematipico ematipico temporarily deployed to aws May 23, 2022 13:33 Inactive
@ematipico
Copy link
Contributor Author

Thanks @MichaReiser , indeed that soft line break was redundant. I removed it.

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Would you mind adding the metrics to the test plan? Looks good.

@ematipico ematipico force-pushed the fix/sequence-expressions-indentation branch from ca26abe to 74888c2 Compare May 23, 2022 14:23
@ematipico ematipico temporarily deployed to aws May 23, 2022 14:23 Inactive
@ematipico ematipico merged commit 33bb5f0 into main May 23, 2022
@ematipico ematipico deleted the fix/sequence-expressions-indentation branch May 23, 2022 15:16
@ematipico
Copy link
Contributor Author

Would you mind adding the metrics to the test plan? Looks good.

Added!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long sequence expressions
4 participants