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

feat(rome_js_formatter): 💡 Prettier Compatibility Metric #2574

Merged
merged 13 commits into from
May 13, 2022

Conversation

IWANABETHATGUY
Copy link
Contributor

@IWANABETHATGUY IWANABETHATGUY commented May 11, 2022

Summary

part of #2555

Test Plan

@IWANABETHATGUY IWANABETHATGUY changed the title refactor: 💡 extract reprot_prettier feat(rome_js_formatter): 💡 Prettier Compatibility Metric May 11, 2022
@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review May 11, 2022 10:19
crates/rome_js_formatter/tests/prettier_tests.rs Outdated Show resolved Hide resolved
crates/rome_js_formatter/tests/prettier_tests.rs Outdated Show resolved Hide resolved
crates/rome_js_formatter/tests/prettier_tests.rs Outdated Show resolved Hide resolved
crates/rome_js_formatter/tests/prettier_tests.rs Outdated Show resolved Hide resolved
@MichaReiser
Copy link
Contributor

Would you mind creating a Gist and sharing the link of the gist as part of your test plan?

@IWANABETHATGUY
Copy link
Contributor Author

Would you mind creating a Gist and sharing the link of the gist as part of your test plan?

no, of course

@IWANABETHATGUY
Copy link
Contributor Author

@IWANABETHATGUY
Copy link
Contributor Author

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.

Looks good. I've a few suggestions regarding the formatting and naming of the metrics.

crates/rome_js_formatter/tests/prettier_tests.rs Outdated Show resolved Hide resolved
crates/rome_js_formatter/tests/prettier_tests.rs Outdated Show resolved Hide resolved
@IWANABETHATGUY
Copy link
Contributor Author

@MichaReiser
Copy link
Contributor

MichaReiser commented May 12, 2022

Awesome. This is getting sooo good!

I find the

File Based Average Prettier Similarity: 69.29%
Line Based Average Prettier Similarity: 64.28%

hard to understand. Which is why I would opt for Prettier similarity for the "line based" and Average prettier similarity for the file based.

I think these terms are more intuitive because the common definition of an average is that you take the value for each file and divide it by the number of files, which is exactly how the metric is defined.

The total Prettier Similarity metric then remains a bit more obscure but that's already the case with the per file on which is why I believe that's fine.

I think it would help to either:

  • Include a link to the issue in the report where people can look up the metric definition
  • Or include the metric definitions in the report.

@IWANABETHATGUY
Copy link
Contributor Author

Awesome. This is getting sooo good!

I find the

File Based Average Prettier Similarity: 69.29%
Line Based Average Prettier Similarity: 64.28%

hard to understand. Which is why I would opt for Prettier similarity for the "line based" and Average prettier similarity for the file based.

I think these terms are more intuitive because the common definition of an average is that you take the value for each file and divide it by the number of files, which is exactly how the metric is defined.

The total Prettier Similarity metric then remains a bit more obscure but that's already the case with the per file on which is why I believe that's fine.

I think it would help to either:

  • Include a link to the issue in the report where people can look up the metric definition
  • Or include the metric definitions in the report.

https://gist.github.com/IWANABETHATGUY/148d1c973aa3e5b604159b6f5890d538

@IWANABETHATGUY
Copy link
Contributor Author

@NicholasLYang , It seems our Ci has failed due to crates/website/playgroud/lib.rs, would you mind making a pull request to fix it?

@MichaReiser
Copy link
Contributor

Awesome. Thank you. This metric will help me a lot when refactoring some of the printer behaviour to track if the changes are improving compatibility or not.

Are you interested in tackling the CI integration (writing the information to a JSON file and having a command to compare the results) next?

@MichaReiser MichaReiser merged commit 5baf791 into rome:main May 13, 2022
@IWANABETHATGUY IWANABETHATGUY deleted the feat/prettier-metric branch May 13, 2022 06:07
@IWANABETHATGUY
Copy link
Contributor Author

Would the json file need the diff info?

@MichaReiser
Copy link
Contributor

I see this as a multip step process, similar to how the parser coverage works

  • The DiffReporter has an option to emit a JSON instead of a md file
  • There's a new xtask command that given two paths to such report JSON files prints a nice table with the diff (see xtask coverage compare)

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.

3 participants