This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
feat(rome_js_formatter): Parenthesize TypeScript types #3108
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
MichaReiser
force-pushed
the
feat/parentheses-transform
branch
from
234c161
to
e6730da
Compare
MichaReiser
force-pushed
the
feat/parenthesise-types
branch
from
cd962f2
to
9ef7297
Compare
Deploying with
|
| Latest commit: |
52386d2
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8259e6f2.tools-8rn.pages.dev |
| Branch Preview URL: | https://feat-parenthesise-types.tools-8rn.pages.dev |
MichaReiser
commented
Aug 25, 2022
| @@ -716,6 +789,86 @@ impl NeedsParentheses for JsAnyAssignmentPattern { | |||
| } | |||
| } | |||
|
|
|||
| impl NeedsParentheses for TsType { | |||
There was a problem hiding this comment.
These union implementation isn't used anywhere but I think it will be useful to remind everyone adding a new TsType to also implement NeedsParentheses
MichaReiser
commented
Aug 25, 2022
| @@ -47,11 +47,11 @@ export type a = | |||
| - // prettier-ignore | |||
There was a problem hiding this comment.
We can't support these prettier ignore comments at the moment (were ignored before). We'll only be able to support these ignore comments with the new comments infrastructure
MichaReiser
commented
Aug 25, 2022
| @@ -11,60 +11,56 @@ impl FormatRule<JsVariableDeclaratorList> for FormatJsVariableDeclaratorList { | |||
| type Context = JsFormatContext; | |||
|
|
|||
| fn fmt(&self, node: &JsVariableDeclaratorList, f: &mut JsFormatter) -> FormatResult<()> { | |||
| let format_inner = format_with(|f| { | |||
| let length = node.len(); | |||
| let length = node.len(); | |||
There was a problem hiding this comment.
I moved the group outside into the FormatJsVariableDeclaration to match prettier's formatting
MichaReiser
changed the title
*Conent APIs to WASM bindings (#3097)
This was referenced Aug 25, 2022
Closed
MichaReiser
force-pushed
the
feat/parenthesise-types
branch
from
9ef7297
to
c7a8c36
Compare
This PR implements the rules for when parentheses around TypeScript types are needed or can be removed without changing the semantics of the program. The majority of the PR is to implement `NeedsParentheses` for every `TsType`. This PR further fixes an instability issue with the syntax rewriter. The rewriter used to remove all whitespace between the `(` paren and the token (or first comment and skipped token trivia). This is necessary or the formatter otherwise inserts an extra blank line before nodes that are parenthesized: ``` a ( Long && Longer && ) ``` becomes ``` a ( Long && Longer && ) ``` Notice, the added new line. What this logic didn't account for is that it should not remove a leading new line before a comment because we want to keep the comment on its own line and this requires that there's a leading new line trivia. The last fix is in the source map that so far assumed that all ranges are added in increasing `end` order ``` correct: 2..3, 2..4, 2..5 (notice how the ranges are sorted by end) incorrect: 2..4, 2..3, 2..5 ``` This PR updates the sorting of the text ranges to also account the end ranges. I added unit tests for all rules where parentheses are required and reviewed the updated snapshots. There are a few new changes but these unrelated to parentheses but instead problems with the formatting of the specific syntax. Average compatibility: 83.70 -> 84.11 Compatible lines: 80.79 -> 81.42
MichaReiser
force-pushed
the
feat/parenthesise-types
branch
from
f960d6b
to
8ae1f8e
Compare
|
!bench_formatter |
Formatter Benchmark Results |
ematipico
approved these changes
Aug 30, 2022
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.
This PR implements the rules for when parentheses around TypeScript types are needed or can be removed without changing the semantics of the program.
The majority of the PR is to implement
NeedsParenthesesfor everyTsType.Tests
I added unit tests for all rules where parentheses are required and reviewed the updated snapshots.
There are a few new changes but these unrelated to parentheses but instead problems with the formatting of the specific syntax.
Average compatibility: 84.00 -> 84.41
Compatible lines: 81.75 -> 82.38