Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use prettier CLI + pretty-quick #823

Merged
merged 1 commit into from Aug 19, 2022
Merged

Use prettier CLI + pretty-quick #823

merged 1 commit into from Aug 19, 2022

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Aug 18, 2022

Avoid too much drift with prettier CLI. Let's see how well this works for us (pretty-quick feels a bit faster)

@render
Copy link

render bot commented Aug 18, 2022

@oliviertassinari oliviertassinari requested a deployment to try-out-bare-prettier - toolpad-db PR #823 August 18, 2022 14:10 — with Render Abandoned
@Janpot Janpot marked this pull request as ready for review August 18, 2022 16:28
@Janpot Janpot requested a review from bytasv August 19, 2022 08:05
@Janpot Janpot merged commit 4aa32bf into master Aug 19, 2022
@Janpot Janpot deleted the try-out-bare-prettier branch August 19, 2022 10:22
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 24, 2022

If it works well, could we replicate the change to mui/material-ui? I think that this mono-repository should be our source of truth for tooling. This would avoid us to have to find how to fix the same bugs in two different ways, as well as help to better stress test the solutions (faster feedback loop).

@Janpot
Copy link
Member Author

Janpot commented Aug 25, 2022

Giving it a try here: mui/material-ui#34062

pretty-quick seems to be about 300ms faster for some reason:

yarn run v1.22.19
$ pretty-quick --ignore-path .eslintignore
🔍  Finding changed files since git revision df891841e9.
🎯  Found 4 changed files.
✅  Everything is awesome!
✨  Done in 0.84s.

vs

yarn run v1.22.19
$ node ./scripts/prettier.js
✨  Done in 1.11s.

The only problem I ran into is the handling of translations: https://github.com/mui/material-ui/blob/df891841e991a4c5e84cd20b79352b18b04fba84/scripts/prettier.js#L64-L67
It seems like we don't enforce any formatting on files coming from crowdin. prettier finds 85 translated files with formatting problems. e.g. there's the codeblock containing purple. A200;. It seems to me it would be better to enforce correct formatting in crowdin (maybe we can block merging these translations if prettier fails?).

Also had this PR open already to fix the yml handling of the current solution mui/material-ui#33980. It was not the first time that I noticed the script has drifted from prettier capabilities, which is the reason why I suggested replacing it with the prettier CLI.

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants