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

test(rome_js_formatter): support report prettier metric as a json file #2626

Merged
merged 7 commits into from
Jun 20, 2022

Conversation

IWANABETHATGUY
Copy link
Contributor

@IWANABETHATGUY IWANABETHATGUY commented May 29, 2022

Summary

part of #2555
To achieve printing diff between main branch and pull request branch, a few steps we need to do list below

  1. save prettier compatibility metric on main branch
  2. switch to pull request branch
  3. save prettier compatibility metric on pull request branch
  4. compare two metrics and echo the diff.

Obviously, Markdown is not a good format for comparing, so we need to add an extra format to save metrics, which is Json
just like Parser conformance

This pull request introduces these two changes:

  1. Add an option to report the prettier compatibility metric in json or markdown format.
  2. Support choosing file_name to save.

Test Plan

No need to test

@IWANABETHATGUY IWANABETHATGUY changed the title chore: 🤖 support report as a json file (rome_js_formatter) 🤖 support report prettier metric as a json file May 29, 2022
@IWANABETHATGUY IWANABETHATGUY changed the title (rome_js_formatter) 🤖 support report prettier metric as a json file (rome_js_formatter): support report prettier metric as a json file May 29, 2022
@ematipico ematipico changed the title (rome_js_formatter): support report prettier metric as a json file test(rome_js_formatter): support report prettier metric as a json file May 30, 2022
@ematipico
Copy link
Contributor

Could you add few lines in the description of the PR about:

  • why you're adding this to the tests and what kind of problem we're trying to solve;
  • a reference to an issue where this new addition was discussed;

@IWANABETHATGUY
Copy link
Contributor Author

Done

Copy link
Contributor

@yassere yassere left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I agree that it makes sense to have a json option so that changes can be compared more easily in CI.

Currently, this PR duplicates a lot of the report calculations, which makes it more likely to fall out of sync if we modify our metrics in the future. Could you extract those calculations so that the report_json and report_markdown methods are only directly responsible for the differences that are specific to that report type?

Maybe there could be a method that produces a PrettierReport that the individual report_X methods know how to print into different formats?

@IWANABETHATGUY
Copy link
Contributor Author

agree

@yassere
Copy link
Contributor

yassere commented May 31, 2022

We could also update the .gitignore to ignore crates/rome_js_formatter/report.* rather than just the markdown file.

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.

Agree with Yasser. I think it would be good to factor out the computation into a helper function that returns the numbers and the markdown/json functions only print the results differently.

@IWANABETHATGUY
Copy link
Contributor Author

I have refactored the code, I think I am ready for reviewing.

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.

Thanks for doing the refactor. That should help us to keep the implementations in sync in the future.

I've a small comment that may help to reduce some code. Let me know if you want to address it before merging or prefer to keep it as is.

@MichaReiser MichaReiser merged commit c05d02b into rome:main Jun 20, 2022
@IWANABETHATGUY IWANABETHATGUY deleted the feat/prettier-metric-json branch June 20, 2022 15:57
@IWANABETHATGUY IWANABETHATGUY restored the feat/prettier-metric-json branch June 21, 2022 16:53
@IWANABETHATGUY IWANABETHATGUY deleted the feat/prettier-metric-json branch June 21, 2022 16:53
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.

4 participants