Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

challenge(formatter): handle line break and comments for array holes #756

Merged
merged 1 commit into from Nov 21, 2023

Conversation

victor-teles
Copy link
Contributor

@victor-teles victor-teles commented Nov 17, 2023

Summary

This PR fixes comments placement for array holes.

partially fixes js/arrays/numbers-with-holes.js

I've to introduced a new entry_with_separator method for less generic line breaking in arrays as holes need it's own logic.

I going to work in line breaking limitation in another PR

Test Plan

Update snapshots

@github-actions github-actions bot added A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages labels Nov 17, 2023
@victor-teles
Copy link
Contributor Author

@biomejs/core-contributors For some reason i have exact match in playground but the snapshot diff fails.

CleanShot 2023-11-17 at 17 10 25

@ematipico
Copy link
Member

@victor-teles can you point us to the prettier implementation? We usually check their source code, to make sure we match their logic.

One more thing, checking the formatted content doesn't help sometimes. You should check the emitted IR.

@victor-teles
Copy link
Contributor Author

victor-teles commented Nov 18, 2023

@ematipico I think prettier don't have a comment handling specific for "array hole". (Maybe I need to do some more research)

I think my solution can be simplified if I can get the get_lines_before method to work for the JsArrayHole node.
At the moment, it's not working because JsArrayHole has no tokens and leading_trivia, Which is used to determine the number of lines.

With it working, I could just keep the following change: crates/biome_js_formatter/src/comments.rs

@ematipico
Copy link
Member

@victor-teles
Copy link
Contributor Author

@ematipico But they don't handle array holes in that file like biome does

@ematipico
Copy link
Member

Thank you for flagging this. You can use #739 to track our divergence, in case you think we should

@victor-teles victor-teles force-pushed the fix/holes-comments branch 3 times, most recently from 286ea91 to b9b05fc Compare November 20, 2023 19:24
@victor-teles victor-teles marked this pull request as ready for review November 21, 2023 14:14
@Conaclos Conaclos merged commit 579b11a into biomejs:main Nov 21, 2023
12 checks passed
@victor-teles victor-teles deleted the fix/holes-comments branch November 21, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Formatter Area: formatter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants