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

test(rome_js_formatter): update prettier tests #2704

Merged
merged 3 commits into from
Jun 14, 2022

Conversation

denbezrukov
Copy link
Contributor

Summary

Update prettier tests and prettier version for a playground.

Prettier after 2.6.0 release treats comment different.

Input

f4 = // Comment 
() => {};

2.6.0 Playground

f4 = // Comment
  () => {};


Rome playground


f4 = () => {}; // Comment

Test Plan

???

@MichaReiser
Copy link
Contributor

Nice. What do you think of changing prettier's trailingComma option to always to match Rome's default behaviour? Would that be possible or is this hard to achieve because of how we run the tests today?

@denbezrukov
Copy link
Contributor Author

Nice. What do you think of changing prettier's trailingComma option to always to match Rome's default behaviour? Would that be possible or is this hard to achieve because of how we run the tests today?

What is Rome's default behaviour for trailingComma? Is it all?

Hmm,
Do you mean it's better to translate prettier's snapshots with different options before extracting them?

@ematipico
Copy link
Contributor

What is Rome's default behaviour for trailingComma? Is it all?

Yes, by default Rome always adds a trailing comma

Hmm,
Do you mean it's better to translate prettier's snapshots with different options before extracting them?

Yes. Rome and Prettier have different defaults

@MichaReiser
Copy link
Contributor

Hmm,
Do you mean it's better to translate prettier's snapshots with different options before extracting them?

That would be awesome if it's doable with a reasonable effort. I'm not familiar with how the test extraction works so I'm unable to assess the effort and maybe it's even best to tackle this in a separate PR.

@denbezrukov
Copy link
Contributor Author

That would be awesome if it's doable with a reasonable effort. I'm not familiar with how the test extraction works so I'm unable to assess the effort and maybe it's even best to tackle this in a separate PR.

Interesting, I think that we can do it. We can try to re-run prettier with options before saving snapshot.

@denbezrukov
Copy link
Contributor Author

@MichaReiser @ematipico
Additional format with prettier before snapshot saving.

925f6a9 - file changes
a0291f3 - snapshot changes

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 to me. I didn't review all snapshot changes but I love it that prettier's snapshots now matches Rome's options.

try {
// We need to reformat prettier snapshot
// because Rome and Prettier have different default options
snapContent = prettier.format(snapContent, config);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome. Something we wanted for a long time but no-one ever looked into it. Thank you so much for updating the tests with matching options.

@MichaReiser MichaReiser merged commit e29549a into rome:main Jun 14, 2022
IWANABETHATGUY pushed a commit to IWANABETHATGUY/tools that referenced this pull request Jun 15, 2022
Co-authored-by: Dominionys <6227442+Dominionys@users.noreply.github.com>
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

3 participants