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

fix(rome_formatter): Group ID in fits function #2739

Merged
merged 2 commits into from
Jun 20, 2022
Merged

Conversation

MichaReiser
Copy link
Contributor

Summary

#2589 introduced GroupIds that add support to refer to a specific group when using if_group_breaks and if_group_fits_on_line. However, the implementation doesn't yet respect the group_id when measuring if a group (or element) fits on a line.

This PR ensures that the printer retrieves the id of the referenced group when measuring if the content of a group fits.

Test Plan

You can see now that the numbers-trailing-comma.js.snap now no longer exceeds the line. The issue was that printing the last element and the , exceeded the print width.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jun 20, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: cd9c033
Status: ✅  Deploy successful!
Preview URL: https://f982176f.tools-8rn.pages.dev
Branch Preview URL: https://fix-group-id-in-fits.tools-8rn.pages.dev

View logs

@MichaReiser MichaReiser force-pushed the fix/group-id-in-fits branch from e4ad862 to b1b3524 Compare June 20, 2022 12:50
@MichaReiser MichaReiser temporarily deployed to aws June 20, 2022 12:50 Inactive
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Can we add a test case in our test suite?

@github-actions
Copy link

github-actions bot commented Jun 20, 2022

@MichaReiser MichaReiser temporarily deployed to aws June 20, 2022 15:32 Inactive
@MichaReiser
Copy link
Contributor Author

Can we add a test case in our test suite?

Excellent idea. I added a printer unit test to make the test independent of any future JS formatter changes.

@MichaReiser MichaReiser merged commit acd0fc8 into main Jun 20, 2022
@MichaReiser MichaReiser deleted the fix/group-id-in-fits branch June 20, 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.

2 participants