Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feature(rome_js_formatter): TS Interface formatting #2773

Merged
merged 2 commits into from
Jun 24, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Jun 24, 2022

Summary

This PR improves the formatting of TS interface declarations.

Note: There's an unresolved stability issue around formatting comments which will require #2768 to be resolved

Test Plan

Verified the updated snapshots manually and compared them with prettier

File Based Average Prettier Similarity: 76.01% -> 76.26%
Line Based Average Prettier Similarity: 70.85% -> 71.23%

@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 10:20 Inactive
@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 10:20 Inactive
@MichaReiser
Copy link
Contributor Author

@ematipico I'll leave it to you to decide if you want to ignore the problematic lines and add it to the Tasks section of #2768 or leave the PR open

@cloudflare-pages
Copy link

cloudflare-pages bot commented Jun 24, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5cb2757
Status: ✅  Deploy successful!
Preview URL: https://2fca09f7.tools-8rn.pages.dev
Branch Preview URL: https://feature-interface-formatting.tools-8rn.pages.dev

View logs

@github-actions
Copy link

github-actions bot commented Jun 24, 2022

@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 11:52 Inactive
@MichaReiser
Copy link
Contributor Author

I resolved the instable comment formatting by manually handling comments inside of the interface formatter. This isn't ideal but lets us move forward

Comment on lines +73 to +74
(_, Some(extends_clause)) => extends_clause.syntax(),
(Some(type_parameters), None) => type_parameters.syntax(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Both None of both _? Just to be consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the first case it can be Some(_) | None, that's why I used _. In the second case it can only ever be None (guaranteed by the match above)

@MichaReiser MichaReiser merged commit 4a68cd2 into main Jun 24, 2022
@MichaReiser MichaReiser deleted the feature/interface-formatting branch June 24, 2022 14:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants