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

fix(rome_js_formatter): empty statements in if/else and do/while #2549

Merged
merged 3 commits into from
May 23, 2022

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented May 6, 2022

Summary

Closes #2448

I cherry-picked the implementation of the former PR, which aligns its behaviour to prettier.

The PR changes how if/else and do/while statements are formatted where the body is an empty statement, by matching Prettier's behaviour:

// before
if(a); else ;

// now
if (a);
else;

Test Plan

New tests to cover the case

@ematipico ematipico temporarily deployed to aws May 6, 2022 15:56 Inactive
@github-actions
Copy link

github-actions bot commented May 6, 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%

@github-actions
Copy link

github-actions bot commented May 6, 2022

@cloudflare-workers-and-pages
Copy link

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

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: d3a539b
Status: ✅  Deploy successful!
Preview URL: https://95f6601a.tools-8rn.pages.dev

View logs

@MichaReiser
Copy link
Contributor

It would help reviewers to add some more context to the PR summary. How does it format empty statements differently than before, in what cases. I otherwise have to open the issue, read through all the discussions or infer the logic from your changes which makes it hard to tell if the logic is working as intended.

@ematipico ematipico temporarily deployed to aws May 23, 2022 10:37 Inactive
@ematipico ematipico changed the title fix(rome_js_formatter): print empty statements differently fix(rome_js_formatter): empty statements in if/else and do/while May 23, 2022
@ematipico
Copy link
Contributor Author

It would help reviewers to add some more context to the PR summary. How does it format empty statements differently than before, in what cases. I otherwise have to open the issue, read through all the discussions or infer the logic from your changes which makes it hard to tell if the logic is working as intended.

I updated the description with a brief example of the changes.

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.

Looks good. Would you mind generating the prettier report and posting how the metric changes with your PR?

crates/rome_js_formatter/src/js/statements/if_statement.rs Outdated Show resolved Hide resolved
@ematipico ematipico temporarily deployed to aws May 23, 2022 11:25 Inactive
@ematipico
Copy link
Contributor Author

Looks good. Would you mind generating the prettier report and posting how the metric changes with your PR?

PR

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

main

**File Based Average Prettier Similarity**: 70.80%  
**Line Based Average Prettier Similarity**: 64.83%  

@ematipico ematipico merged commit 8be953a into main May 23, 2022
@ematipico ematipico deleted the fix/do-while-empty-body-2 branch May 23, 2022 12:08
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.

🐛 loop/if with empty statement body
3 participants