feat(rome_js_formatter): jestEach template literals #3308 #3582
Conversation
✅ Deploy Preview for docs-rometools canceled.Built without sensitive environment variables
|
denbezrukov
force-pushed
the
jest-each
branch
2 times, most recently
from
0dbece5
to
119ea23
Compare
| text.push_str(&padding); | ||
| } | ||
|
|
||
| text.push(' '); |
There was a problem hiding this comment.
I was wondering if we could use space instead of ' '
There was a problem hiding this comment.
space won't work because our printer de-duplicates spaces (space(), space() gets printed as a single space).
You can try to use align instead but I think using ' ' is fine.
There was a problem hiding this comment.
Awesome work. I have some small nit comments.
One thing we need to look into is guarding against skipped token trivia on elements that you remove. Because that would result in the skipped token trivia being emitted before the template.
I recommend testing for skipped token trivia in the is_test_each_pattern and bail out if it has any.
The alternative is to use the FormatError::PoorLayout and catch the template formatting error to fall back to the default formatting. I think explicitly testing is nicer and the overhead should be reasonable (considering the complexity of doing the formatting).
| text.push_str(&padding); | ||
| } | ||
|
|
||
| text.push(' '); |
There was a problem hiding this comment.
space won't work because our printer de-duplicates spaces (space(), space() gets printed as a single space).
You can try to use align instead but I think using ' ' is fine.
denbezrukov
force-pushed
the
jest-each
branch
3 times, most recently
from
4b329e9
to
9bab47f
Compare
Thank you for your review! I added |
| if leading_trivia.pieces().next().is_some() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Is this the test asserting if it has any skipped token trivia?
That's probably going to work but using leading_trivia.has_skipped() is more explicit and easier to understand to readers.
|
!bench_formatter |
denbezrukov
force-pushed
the
jest-each
branch
from
9bab47f
to
adc43fc
Compare
Formatter Benchmark Results |
denbezrukov
force-pushed
the
jest-each
branch
from
5fe4956
to
30cb7b9
Compare
* upstream/main: (45 commits) website(docs): set `color-scheme` on the root element (rome#3721) feat(rome_analyze): add a warning for unused suppression comments (rome#3718) feat(rome_js_analyze): Implement prefer-numeric-literals lint (rome#3558) feat(rome_js_formatter): jestEach template literals rome#3308 (rome#3582) doc(website): Add context about Romes philosophy (rome#3714) fix(rome_js_formatter): Single-line comment below a JSX prop triggers… (rome#3641) test(rome_js_formatter): update prettier tests (rome#3684) fix(rome_js_parser): improve await handling in non-async context (rome#3573) fix(rome_js_parser): improve yield parsing in non generator function (rome#3622) More playground polish Fix backgrounds Fix height Align docs.rome.tools with rome.tools Reenable compression Add missing width website(docs): More playground IDE features (rome#3711) fix(rome_js_formatter): new expression attribute (rome#3686) docs(website): added checkbox to toggle linter in playground (rome#3699) website(docs): More website tweaks (rome#3707) website(docs): Add default layout property (rome#3705) ...
Summary
Close #3308
Test Plan
cargo test -p rome_js_formatter