feat(rome_js_formatter): Conditional expression formatting #3024
Conversation
f7c6b7e to
7102fcf
Compare
Deploying with
|
| Latest commit: |
ab1d20c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://98554c7b.tools-8rn.pages.dev |
| Branch Preview URL: | https://feat-conditional-expression.tools-8rn.pages.dev |
| @@ -49,7 +50,7 @@ impl<'fmt, Context> Argument<'fmt, Context> { | |||
| } | |||
|
|
|||
| /// Formats the value stored by this argument using the given formatter. | |||
| #[inline] | |||
| #[inline(always)] | |||
There was a problem hiding this comment.
I ran into issues with stack overflows on the playground in debug builds. Adding inline(always) ensures that this "trivial" calls are also inlined in debug mode which results in nicer stack traces
| @@ -333,7 +333,7 @@ impl<'a> Printer<'a> { | |||
| // If the indentation level has changed since these line suffixes were queued, | |||
| // insert a line break before to push the comments into the new indent block | |||
| // SAFETY: Indexing into line_suffixes is guarded by the above call to is_empty | |||
| let has_line_break = self.state.line_suffixes[0].args.indent < args.indent; | |||
| let has_line_break = self.state.line_suffixes[0].args.indent.level() < args.indent.level(); | |||
There was a problem hiding this comment.
Comparing the align isn't necessary as it turns out, this hack is only for moving comments after a (, [, or { into the indent.
| + } | ||
| + } | ||
| +), | ||
| + ( |
There was a problem hiding this comment.
This is an issue with parenthesized expressions
| @@ -148,228 +130,165 @@ a | |||
| - </Content> | |||
| - </Message> | |||
| - ); | |||
| +const StorybookLoader = ({ match }) => ( | |||
| + match.params.storyId === "button" | |||
There was a problem hiding this comment.
JSX special case is intentionally excluded from this PR
| } | ||
|
|
||
| function foo() { | ||
| - void (( | ||
| - coooooooooooooooooooooooooooooooooooooooooooooooooooond | ||
| - ? baaaaaaaaaaaaaaaaaaaaar | ||
| + void ( | ||
| + (coooooooooooooooooooooooooooooooooooooooooooooooooooond |
There was a problem hiding this comment.
I only implemented the parentheses handling for expressions but not types. It will be easier to address this with an overall solution that avoids "resolving" expressions everywhere.
7102fcf to
c72beb3
Compare
|
!bench_formatter |
Formatter Benchmark Results |
I'm surprised this isn't worse. I guess, there aren't so many conditional expressions in our benchmark files |
c72beb3 to
f10e508
Compare
f10e508 to
863b589
Compare
| @@ -1,39 +0,0 @@ | |||
| use crate::context::QuoteStyle; | |||
There was a problem hiding this comment.
I had to move this is a manually created file and re-generating the formatter deletes the file. It's now part of utils/jsx
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
863b589 to
94de173
Compare
| @@ -689,46 +688,6 @@ impl Indention { | |||
| } | |||
| } | |||
|
|
|||
| /// Compares two [Indent]s by first comparing the indent level and, if equal, comparing the `align`. | |||
| /// This implementation assumes that `width(align) < width(1-level)`. | |||
| impl PartialOrd for Indention { | |||
There was a problem hiding this comment.
It's no longer needed, comparing the indent level is sufficient.
crates/rome_js_formatter/src/js/expressions/arrow_function_expression.rs
Outdated
Show resolved
Hide resolved
| f, | ||
| [group(&soft_line_indent_or_space(&format_with(|f| { | ||
| if should_add_parens { | ||
| write!(f, [if_group_fits_on_line(&text("("))])?; |
There was a problem hiding this comment.
nit: just noticed this now but.. shouldn't this be if_group_fits_in_line?
There was a problem hiding this comment.
My understanding is:
fits in... you can stuff it into something.fits onYou can put it onto something?
That's why I think on is more appropriate. But maybe that's just due to my German background.
| let argument = match parent.kind() { | ||
| JsSyntaxKind::JS_INITIALIZER_CLAUSE => { | ||
| let initializer = JsInitializerClause::unwrap_cast(parent); | ||
| initializer.expression().ok().map(JsAnyExpression::from) |
There was a problem hiding this comment.
I am not sure sure we should "mute" a SyntaxResult. Is there a specific reason for doing so?
There was a problem hiding this comment.
It's safe to do so here because the function will return the "safe" default of false if the returned value from here is Err.
The errors get handled as part of the parent node.
| let arrow = JsArrowFunctionExpression::unwrap_cast(parent.clone()); | ||
|
|
||
| match arrow.body() { | ||
| Err(_) => false, |
There was a problem hiding this comment.
I am not sure we should mute an error. I think we should bounce it to the parent
There was a problem hiding this comment.
This code is testing if the arrow.body is the direct parent because it may only then be necessary to add parentheses.
That also means that returning false is safe if it's an Err because it's then guaranteed that the conditional is for certain not the body (because it would then not be missing)
94de173 to
cf30755
Compare
68f3d30 to
a53f0d5
Compare
| assertion_line: 271 | ||
| info: | ||
| test_file: js/unary-expression/urnary_expression.js |
There was a problem hiding this comment.
I believe these changes can be reverted
Refactor conditional formatting to better make use of the new formatter infrastructure
a53f0d5 to
ab1d20c
Compare
Summary
This PR implements conditional expression formatting similar to Prettier with the exception of the special handling of conditional expressions containing JSX (PR is already huge).
Test Plan
I manually reviewed the snapshot changes. Some changes may appear as regressions but are mainly due to other syntax formatting (binary expression, call arguments). I ignored comments in this PR (except to make sure comments are stable) as we plan to refactor comments.
File Based Average Prettier Similarity: 78.85% -> 79.72%
Line Based Average Prettier Similarity: 74.25% -> 76.49%