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

fix(formatter): Stable member chain printing #2582

Merged
merged 4 commits into from
May 13, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented May 13, 2022

This PR simplifies the member printing logic to make sure its output is stable.
Interestingly, this is slightly improving the prettier compatibility

Before:
**File Based Average Prettier Similarity**: 69.29%  
**Line Based Average Prettier Similarity**: 64.28%  

After:
**File Based Average Prettier Similarity**: 69.39%  
**Line Based Average Prettier Similarity**: 64.40%  

But unblocks my work on refactoring the printer behaviour (which seems to trigger this now a lot), and #2458 . The work on improving the formatting is tracked in #2421

Test Plan

I rebased #2458 (need to find a way to push this change) and the tests now pass.

This PR simplifies the member printing logic to make sure its output is stable.
This results in a small regression

```
Before:
**File Based Average Prettier Similarity**: 69.29%
**Line Based Average Prettier Similarity**: 64.28%

After:
**File Based Average Prettier Similarity**: 69.13%
**Line Based Average Prettier Similarity**: 64.06%
```

But unblocks my work on refactoring the printer behaviour (which seems to trigger this now a lot), and #2458
@cloudflare-workers-and-pages
Copy link

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

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5e8b0a2
Status: ✅  Deploy successful!
Preview URL: https://e4a1592b.tools-8rn.pages.dev

View logs

@MichaReiser MichaReiser temporarily deployed to aws May 13, 2022 13:41 Inactive
@github-actions
Copy link

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 13, 2022

@MichaReiser MichaReiser requested a review from leops May 13, 2022 13:48
Copy link
Contributor

@leops leops left a comment

Choose a reason for hiding this comment

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

I does result in quite a few regressions but I guess that's fine as the intent is to go back to the chain printing later on and fully align it with Prettier when we have the appropriate IR to do so

@@ -132,53 +103,16 @@ impl<'f> Groups<'f> {
/// Format groups on multiple lines
pub fn into_joined_hard_line_groups(self) -> FormatElement {
let formatted_groups = self.into_formatted_groups();
join_elements(hard_line_break(), formatted_groups)
concat_elements(formatted_groups)
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is now doing the same thing as into_format_elements, is there any point in keeping it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted this change with the added benefit that there are now fewer regressions (overall even an increase in prettier compatibility)

@MichaReiser MichaReiser temporarily deployed to aws May 13, 2022 14:08 Inactive
@MichaReiser MichaReiser temporarily deployed to aws May 13, 2022 14:13 Inactive
@MichaReiser MichaReiser temporarily deployed to aws May 13, 2022 14:14 Inactive
@MichaReiser
Copy link
Contributor Author

@cpojer You should be able to land your PR after this :) I think it's improving our new metric by about 1%!

@MichaReiser
Copy link
Contributor Author

I does result in quite a few regressions but I guess that's fine as the intent is to go back to the chain printing later on and fully align it with Prettier when we have the appropriate IR to do so

Yeah, unfortunately. But it's sometimes necessary to go a few steps back to be able to move forward again. I think this is one such situation (I still have to fight with some other unstable results but at least fewer of them)

@MichaReiser MichaReiser temporarily deployed to aws May 13, 2022 14:18 Inactive
@MichaReiser MichaReiser merged commit d78e802 into main May 13, 2022
@MichaReiser MichaReiser deleted the fix/stable-member-printing branch May 13, 2022 14:29
@ematipico
Copy link
Contributor

I am not sure if this was the correct way to go. I hope it is. It seems that lot of logic taken from prettier was removed.

@MichaReiser
Copy link
Contributor Author

I agree that this hasn't been ideal but it was, in my view necessary, to make progress on changing the underlying printer architecture without being stuck for days (It still took me 1.5 weeks). It should be easier to re-add this logic now, that the printer works the same as prettier.

Something I noticed is that we often form groups in the exact opposite order than prettier. For example, prettier groups left && where we group && right. These are things that we need to re-visit to align our formatting.

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.

3 participants