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

fix(rome_js_formatter): regression of call arguments #2812

Merged
merged 5 commits into from
Jul 4, 2022

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Jul 4, 2022

Summary

This PR fixed #2720

The new logic checks if any arguments breaks and if so, it does a custom printing.

Doing so, it triggered a regression of the new code that we didn't see before:

This snippet:

test.expect(t => {
	t.true(a);
}, false /* inline comment */ );

Would be formatted

test.expect ((t) => {
	t.true(a);
}, false /* inline comment */ );

Note the space added before the first (. This regression is caused by a bug inside the function write_space_between_comment_and_token, which triggers a false positive condition and it adds a space before the token.

I will have to look into it later. In order to avoid to commit wrong code, I changed the snippet that was causing the false positive. Tracked in #2814

Another regression that is triggered by our printer is already tracked in #2815 . This issue is caused by how best_fitting! prints the second alternative.

I would like to shut down this reformat issue, because the regression that is fixed in this PR is actually more important then the format issue, I think.

Test Plan

cargo test and checked that the regression is not fixed

PR
File Based Average Prettier Similarity: 77.44%
Line Based Average Prettier Similarity: 72.72%

main
File Based Average Prettier Similarity: 77.33%
Line Based Average Prettier Similarity: 72.61%

@ematipico ematipico temporarily deployed to aws July 4, 2022 11:28 Inactive
@ematipico ematipico requested review from yassere and leops July 4, 2022 11:28
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 4, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6a92176
Status: ✅  Deploy successful!
Preview URL: https://8627c3b8.tools-8rn.pages.dev
Branch Preview URL: https://fix-will-break-call-argument.tools-8rn.pages.dev

View logs

@github-actions
Copy link

github-actions bot commented Jul 4, 2022

@ematipico ematipico temporarily deployed to aws July 4, 2022 12:08 Inactive
@ematipico ematipico temporarily deployed to aws July 4, 2022 12:41 Inactive
write!(will_break_buffer, [element]).ok();
will_break_buffer.will_break()
});
let any_breaks = arguments_break.any(|will_break| will_break);
Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly calling .any will mutate the arguments_break iterator in place, so all the iterators items consumed to initialize any_breaks will not be processed by the next call to .any to initialize an_argument_breaks, is this really what's intended here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's really a good catch, and I was wondering why I could not replicate a reformat issue that I was encountering some time ago.

@ematipico ematipico temporarily deployed to aws July 4, 2022 14:28 Inactive
@ematipico ematipico requested a review from leops July 4, 2022 14:28
@ematipico ematipico requested a review from leops July 4, 2022 15:19
@ematipico ematipico temporarily deployed to aws July 4, 2022 15:19 Inactive
@ematipico ematipico merged commit 6c146b8 into main Jul 4, 2022
@ematipico ematipico deleted the fix/will-break-call-arguments branch July 4, 2022 15:41
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.

Fix regression on call arguments with will_break
3 participants