feature(formatter): Fill separators #2609
Conversation
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
Deploying with
|
| Latest commit: |
a0fc08a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2b826283.tools-8rn.pages.dev |
crates/rome_js_formatter/src/lib.rs
Outdated
| "# | ||
| ); | ||
|
|
||
| println!("{}", result.as_code()); |
There was a problem hiding this comment.
Maybe assert would be better here, since the function name is quick_test.
There was a problem hiding this comment.
True, we should revert this line
There was a problem hiding this comment.
It's not super important considering the quick_test function is ignored by default. It's just a way to test code quickly.
There was a problem hiding this comment.
Apologies Nicholas, but even though this test is ignored, I use this test to check test things locally using my IDE (and probably other people too). If we remove the assertion, I will have to add it again.
| /// let b = "Please do not commit memory bugs such as segfaults, buffer overflows, etc. otherwise you "; | ||
| /// let c = "<em>will</em>"; | ||
| /// let d = " be reprimanded"; | ||
| /// let expr = fill_elements(empty_element(), [token(a), token(b), token(c), token(d)]); |
There was a problem hiding this comment.
Please create a test with a spacer that has different "flat" and "expanded" representations and make sure the printer prints all these cases correctly
- item, spacer, and next item fit
- item and spacer fit, next item doesn't
- only item fits
There was a problem hiding this comment.
I asked this in the JSX PR. What should the result be if the separator is printed differently? Because the fill printing should handle line breaking, not the separators themselves. Perhaps we should only allow non breaking separators?
There was a problem hiding this comment.
Yeah, in fact, reading the fill printer code, at no point would we print a separator in expanded mode. Either it fits, in which case we print it flat, or we insert a hard line break. In theory we could replace that with printing the separator in expanded mode, but that'd mean users would have to provide a separator that breaks.
There was a problem hiding this comment.
Sorry, I didn't see that comment.
Yes, we would need to change the logic inside of the printer to align it with Prettier's behaviour. The same as you did by allowing any other separator than space.
I quickly glanced through Prettier's fill use cases and the usage for arrays is interesting.
You can see that there's a use case where they use a hardline separator if the next element is separated by an empty line or if the element has a comment and a softline (softline_or_space) otherwise. This is something that we couldn't support with our current design because we only support a single separator.
Simply relying on the flat and expanded mode rather than hardcoding a line break also feels natural. It makes use of the fundamental principals of the printer and should also not be too hard to add. It should only require replacing the hard_line_break with the separator.
| /// .as_code() | ||
| /// ) | ||
| /// ``` | ||
| pub fn fill_elements<TSep: Into<FormatElement>>( |
There was a problem hiding this comment.
Should TSep be Separator?
There was a problem hiding this comment.
I stole this from one of the other functions. I can rename it if necessary
NicholasLYang
force-pushed
the
feature/fill-separators
branch
from
15d2f8d
to
2989555
Compare
| fill.list.content.iter().any(FormatElement::will_break) | ||
| || fill.separator.will_break() |
There was a problem hiding this comment.
Nit. Should be safe to remove the separator from here as it should never break in flat mode (or using fill is pointless)
…h no spaces in between
…esizing jsx tag expressions, updated tests.
NicholasLYang
force-pushed
the
feature/fill-separators
branch
from
88e89d2
to
4a48575
Compare
Summary
Adding a separator option for fill printing. Allows for JSX child list formatting as prettier fill prints JSX children with no spaces in between, when the children are a mixture of text and tags.
Test Plan
Added a doc test here. Existing tests for fill printing should still apply as well.