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

feat(rome_formatter): predictable order of modifiers #2196

Merged
merged 3 commits into from
Mar 3, 2022

Conversation

ematipico
Copy link
Contributor

Summary

This PR implements a predictable ordering of modifiers inside the formatter.

Test Plan

Added few test to cover them. Will need to more once declare in stable

@cloudflare-pages
Copy link

cloudflare-pages bot commented Mar 2, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6a99459
Status:⚡️  Build in progress...

View logs

@github-actions
Copy link

github-actions bot commented Mar 2, 2022

Parser conformance results on ubuntu-latest

ts/babel

Test result main count This PR count Difference
Total 584 584 0
Passed 496 496 0
Failed 88 88 0
Panics 0 0 0
Coverage 84.93% 84.93% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 15983 15983 0
Passed 12082 12082 0
Failed 3901 3901 0
Panics 0 0 0
Coverage 75.59% 75.59% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 39 39 0
Passed 0 0 0
Failed 39 39 0
Panics 0 0 0
Coverage 0.00% 0.00% 0.00%

js/262

Test result main count This PR count Difference
Total 45250 45250 0
Passed 44128 44128 0
Failed 1122 1122 0
Panics 0 0 0
Coverage 97.52% 97.52% 0.00%

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I have a few small comments regarding the implementation

However, the bigger question is if this is something the formatter should be opinionated about or not. Prettier isn't.

crates/rome_formatter/src/utils/mod.rs Outdated Show resolved Hide resolved
crates/rome_formatter/src/utils/mod.rs Outdated Show resolved Hide resolved
crates/rome_formatter/src/utils/mod.rs Outdated Show resolved Hide resolved
crates/rome_formatter/src/utils/mod.rs Outdated Show resolved Hide resolved
crates/rome_formatter/src/utils/mod.rs Outdated Show resolved Hide resolved
crates/rome_formatter/src/utils/mod.rs Outdated Show resolved Hide resolved
@ematipico
Copy link
Contributor Author

I have a few small comments regarding the implementation

However, the bigger question is if this is something the formatter should be opinionated about or not. Prettier isn't.

I am not sure if I'm seeing doable, but Prettier is quite opinionated: check their playground

@ematipico ematipico requested a review from leops March 3, 2022 09:53
@ematipico ematipico force-pushed the feature/format-class-members branch 2 times, most recently from d09d7ac to db5a945 Compare March 3, 2022 11:02
crates/rslint_parser/src/ast/modifier_ext.rs Outdated Show resolved Hide resolved
crates/rslint_parser/src/ast/modifier_ext.rs Outdated Show resolved Hide resolved
@ematipico ematipico force-pushed the feature/format-class-members branch from db5a945 to 96e77dd Compare March 3, 2022 12:17
@ematipico ematipico force-pushed the feature/format-class-members branch from 96e77dd to 6a99459 Compare March 3, 2022 13:57
@ematipico ematipico merged commit bfaaef3 into main Mar 3, 2022
@ematipico ematipico deleted the feature/format-class-members branch March 3, 2022 14:01
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

4 participants