fix(rome_js_formatter): change parenthesized expression formatting #2636
Conversation
| if matches!(parent.kind(), JsSyntaxKind::JS_RETURN_STATEMENT) { | ||
| true | ||
| } else if matches!(parent.kind(), JsSyntaxKind::JS_PARENTHESIZED_EXPRESSION) { | ||
| // In case we are inside a sequence expression, we have to go up a level and see the great parent. | ||
| // Arrow function body and return statements applying indentation for us, so we signal the | ||
| // sequence expression to not add other indentation levels | ||
| let great_parent = parent.parent().map(|gp| gp.kind()); | ||
|
|
||
| matches!( | ||
| great_parent, | ||
| Some( | ||
| JsSyntaxKind::JS_ARROW_FUNCTION_EXPRESSION | ||
| | JsSyntaxKind::JS_RETURN_STATEMENT | ||
| | JsSyntaxKind::JS_PROPERTY_OBJECT_MEMBER | ||
| ) | ||
| ) | ||
| } else { | ||
| false | ||
| } | ||
| matches!( | ||
| parent.kind(), | ||
| JsSyntaxKind::JS_RETURN_STATEMENT | JsSyntaxKind::JS_PARENTHESIZED_EXPRESSION | ||
| ) |
There was a problem hiding this comment.
I’m uncertain about this change, but it seems to increase Prettier alignment in our existing snapshots.
@ematipico What sort of cases was this condition intended to handle?
There was a problem hiding this comment.
Cases like this, but as you're changing the logic of the parenthesis, maybe it's not needed anymore.
| something: (____________first, | ||
| ____________second, | ||
| ____________third, | ||
| ____________third, | ||
| ____________third, | ||
| ____________third, | ||
| ____________third), | ||
| }; |
There was a problem hiding this comment.
This isn’t really an improvement, but trying to fix it in this PR could conflict with #2627
Our current formatting for this already doesn’t match Prettier’s though.
| `111111111 222222222 333333333 444444444 555555555 666666666 777777777 ${(1, | ||
| 2)}`; |
There was a problem hiding this comment.
This isn’t an improvement, but this might be better handled as part of separate PR that brings template literal formatting into closer alignment with Prettier.
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
💥 Failed to Panic (1):ts/babel
ts/microsoft
|
| let expression_kind = node.expression()?.syntax().kind(); | ||
|
|
||
| let requires_indent = match expression_kind { | ||
| // Never block indent a parenthesized multiline string literal | ||
| JsSyntaxKind::JS_STRING_LITERAL_EXPRESSION => false, | ||
| // Only soft block ident a parenthesized sequence expression when it's | ||
| // the child of specific node types | ||
| JsSyntaxKind::JS_SEQUENCE_EXPRESSION => { | ||
| let parent_kind = node.syntax().parent().map(|p| p.kind()); | ||
|
|
||
| matches!( | ||
| parent_kind, | ||
| Some(JsSyntaxKind::JS_RETURN_STATEMENT) | ||
| | Some(JsSyntaxKind::JS_UNARY_EXPRESSION) | ||
| | Some(JsSyntaxKind::JS_ARROW_FUNCTION_EXPRESSION) | ||
| ) | ||
| } | ||
| _ => true, | ||
| }; | ||
| Ok(requires_indent) |
There was a problem hiding this comment.
Nit: It should be possible to match directly on the node rather than going over syntax
match node.expression() {
JsAnyExpression::JsStringLiteralExpression(_) => false,
...
}
| (____________first, | ||
| ____________second, |
There was a problem hiding this comment.
Wasn't this case handled by the code that you have now removed (and mentioned ema?)
There was a problem hiding this comment.
In this case, Prettier would just omit the parentheses. But if you force the parentheses to remain like in your original example, Prettier doesn't block indent the sequence expression. The indentation in your example (copied below) came from the binary expression.
(aaaaaaaaaaaaaaaaaaaaaaaaa +
bbbbbbbbbbbbbbbbbbbbbbbbb +
ccccccccccccccccccccccccc +
ddddddddddddddddddddddddd +
eeeeeeeeeeeeeeeeeeeeeeeee,
"test")();
Another demonstration is when you assign a sequence expression. I'll add a test case for that.
Before this PR:
foo =
(
____________first,
____________second,
____________third,
____________third,
____________third,
____________third,
____________third
);After this PR and also Prettier:
foo =
(____________first,
____________second,
____________third,
____________third,
____________third,
____________third,
____________third);There was a problem hiding this comment.
Yeah, I don't know. Given what we have and what prettier does, do we really need to match it? Doesn't seem that good honestly
There was a problem hiding this comment.
I"m always in favor of deleting code :D I don't think it's that terrible, nice and compact. But to phrase this differently. I don't think it's that bad that it's reason enough for us to diverge, especially if it means we can delete code.
There was a problem hiding this comment.
I expect that this sort of code is uncommon enough in the wild that we don't need to be terribly concerned with how it looks, so I'm also in favor of just simplifying our rules here and matching Prettier. If we ever discover enough of an impact on real-world projects, we can reconsider in the future.
It's not very nice until I resolve the reformat instability. 😅 |
|
@yassere what's the status of this PR? It hasn't had any updates for more than a month |
Summary
This PR increases Prettier alignment by:
Closes #2427
TODO: This introduces a
check_reformatinstability forbinaryish_expressionandlogical_expressionthat I still need to troubleshoot.Test Plan
Confirm snapshots move closer to Prettier output.