This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
feat(rome_js_formatter): call arguments #2711
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
|
!bench_formatter |
Deploying with
|
| Latest commit: |
15bcc1d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5dde7b29.tools-8rn.pages.dev |
| Branch Preview URL: | https://feat-call-arguments.tools-8rn.pages.dev |
Formatter Benchmark Results |
MichaReiser
reviewed
Jun 15, 2022
| @@ -24,7 +25,7 @@ expect(bifornCringerMoshedPerplexSawder.getLongArrayOfNumbers()).toEqual( | |||
| ```js | |||
| expect(bifornCringerMoshedPerplexSawder.getArrayOfNumbers()).toEqual([ | |||
| 1, 2, 3, 4, 5, | |||
| ],); | |||
| }, | ||
| "letters:", | ||
| ); | ||
| ["a", "b", "c"].reduce(function (item, thing) { |
MichaReiser
reviewed
Jun 15, 2022
ematipico
force-pushed
the
feat/call-arguments
branch
3 times, most recently
from
80a7be4 to
ac776d2
Compare
ematipico
force-pushed
the
feat/call-arguments
branch
from
ac776d2 to
b4b941a
Compare
|
!bench_formatter |
Formatter Benchmark Results |
MichaReiser
reviewed
Jun 16, 2022
MichaReiser
reviewed
Jun 16, 2022
MichaReiser
reviewed
Jun 16, 2022
Comment on lines
55
to
62
| format_with(|f| { | ||
| // we don't want to print the trailing separator, so if it's present, we replace it | ||
| // with an empty element | ||
| if let Some(separator) = second_argument.trailing_separator()? { | ||
| return write!(f, [format_removed(separator)]); | ||
| } | ||
|
|
||
| if token_has_comments(&l_paren_token?) || token_has_comments(&r_paren_token?) { | ||
| return Ok(false); | ||
| Ok(()) |
There was a problem hiding this comment.
Nit: I personally prefer to split the write into multiple blocks if I have conditional logic:
write!(f, [l_paren_token.format(), ..., second_argument.node().format()])?;
if let Some(separator) = second_argument.trailing_separator()? {
write!(f, [format_removed(separator)])?;
}
return write!(f, [r_paren_token.format()]);
There was a problem hiding this comment.
It's an habit coming from the old API I suppose :)
MichaReiser
reviewed
Jun 16, 2022
MichaReiser
reviewed
Jun 16, 2022
MichaReiser
reviewed
Jun 16, 2022
There was a problem hiding this comment.
This is great work @ematipico ! I've a few smaller code suggestions but that's it.
My main question now is how we go about to land this since this PR depends on my PR which isn't handling split_trivia correctly yet.
| /// Whether the node contains trailing comments. | ||
| pub fn has_trailing_comments(&self) -> bool { | ||
| self.last_token() | ||
| .map_or(false, |tok| tok.has_trailing_comments()) | ||
| } | ||
|
|
||
| /// Whether the last token of a node has comments (leading or trailing) | ||
| pub fn last_token_has_comments(&self) -> bool { |
There was a problem hiding this comment.
Unrelated to this line: You brought up the issue with comments on discord the other day. Do these helpers address the issue you raised on discord?
There was a problem hiding this comment.
Yes, this was the solution I was looking for. It might be an edge case or not, but for now I decided to applying it only to this particular case.
|
Compatibility: After File Based Average Prettier Similarity: 73.76% Notable remaining differences: Line break before / after js/arrow-call/arrow_call.js it("mocks regexp instances", () => {
- expect(() =>
- moduleMocker.generateFromMetadata(moduleMocker.getMetadata(/a/)),
+ expect(
+ () => moduleMocker.generateFromMetadata(moduleMocker.getMetadata(/a/)),
).not.toThrow();
});object arguments -deepCopyAndAsyncMapLeavesA(
- { source: sourceValue, destination: destination[sourceKey] },
- { valueMapper, overwriteExistingKeys },
-);
+deepCopyAndAsyncMapLeavesA({
+ source: sourceValue,
+ destination: destination[sourceKey],
+}, { valueMapper, overwriteExistingKeys });Hook formatting js/break-calls/react.js+ const { firstName, lastName } = useMemo((
+ aaa,
+ bbb,
+ ccc,
+ ddd,
+ eee,
+ fff,
+ ggg,
+ hhh,
+ iii,
+ jjj,
+ kkk,
+ ) => func(aaa, bbb, ccc, ddd, eee, fff, ggg, hhh, iii, jjj, kkk), [
+ foo,
+ bar,
+ baz,
+ ]);
}
``
# js/comments/arrow.js
also js/comments/function-declaration.js and js/empty-paren-comment/empty_paren_comment.js
Comments formatting (Let's see if my latest PR's help with this one)-const fn = (/*event, data*/) => doSomething();
+const fn = (/*event, data*/ ) => doSomething();
-const fn2 = (/*event, data*/) => doSomething(anything);
+const fn2 = (/*event, data*/ ) => doSomething(anything);I don't mind if we fix some of them later but I think we need to have a closer look why the spacing of comments is now broken. |
MichaReiser
reviewed
Jun 16, 2022
MichaReiser
approved these changes
Jun 16, 2022
crates/rome_js_formatter/tests/specs/js/module/arrow/arrow_nested.js.snap
Show resolved
Hide resolved
MichaReiser
reviewed
Jun 16, 2022
ematipico
force-pushed
the
feat/call-arguments
branch
from
79d601e to
15bcc1d
Compare
Introduces a new reference counted `FormatElement`. Useful for situation where the same content is part of the resulting IR twice but with different parent formatting.
ematipico
force-pushed
the
feat/call-arguments
branch
from
15bcc1d to
9657acb
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR closes #2421 and closes #2439
This PR requires #2685 in order to be merged.
The implementation of the formatting requires the usage of
best_fitting, which will impact the performance of formatter, in some way.The usage of
best_fittingalso doesn't play well when usingdbg_write, which is also understandable.The implementation is a port of Prettier's algorithm: https://github.dev/prettier/prettier/blob/main/src/language-js/print/call-arguments.js
The implementation doesn't cover ALL the cases, as we don't have a yet a good API/builder for
will_break, which means that some cases might not be covered.The final version of the implementation now surgically caches the node/tokens only when we know they need to be printed multiple times. This was a lesson learned, where it's not good doing an early caching regardless, because this was impacting the performance in visible way.
This PR also fixes #2676, the trailing comma is manually added only in the cases where we know there's a group that might break.
Here's the work that has been done:
format_delimited, making it a standalone making it a standalone utility. This was needed because there were case where I needed manually replicate the delimiting logic, by customizing the content. Also, I needed a way to cache the delimiters and print them in different positions based on the condition.Regressions
A known regression is the absence of
will_break, which in turns formats anything with comments in the wrong way. Prettier useswillBreakto check if some arguments break, and if so, then the formatting is different. Will break is fundamental because comments (line suffixes to be more precise) mark an element as "yes, it will break".So a code like this:
Will print like this:
A workaround might be to actually check if each argument has comments, but I am not sure how the outcome is going to be for arguments that have deep nested comments.
Test Plan
I added new test cases, mostly edge cases. The rest of the formatting can be seen inside the prettier's test suite - I believe they speak by themselves.
If you feel the need of more tests, please let me know.
This PR brings us closer to Prettier
main
File Based Average Prettier Similarity: 73.25%
Line Based Average Prettier Similarity: 67.60%
PR:
File Based Average Prettier Similarity: 73.61%
Line Based Average Prettier Similarity: 68.79%
I hoped in an higher number but it's still something :)