-
Notifications
You must be signed in to change notification settings - Fork 202
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
Switch to locally available prettier for pre-commit hook #4749
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Excellent to skip a redundant installation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice that we're reducing a dependency here! However, when I change one file and run ov just
, the output that it produces is quite large because it also shows all the unchanged files (i.e. every file it scans). Is there a way to adjust this behavior? Maybe run it on each file individually, or grep
the output while passing through the error code?
.pre-commit-config.yaml
Outdated
"types": [text] | ||
language: system | ||
pass_filenames: true | ||
entry: pnpm exec prettier --write --ignore-unknown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The eslint
hook below uses bash -c 'pnpm run ...'
, do we need to make the same consideration here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why those use bash -c
but doing so prevents the hook from being able to pass filenames, which is critical to avoid prettier checking every single file in the repo during commit, rather than just those that have changed. The former takes ages and only needs to happen when running with --all-files
as with ov just lint
(which is used in CI).
@AetherUnbound I was able to reproduce what you said, I'm not sure why I didn't see it before. Adding
|
@AetherUnbound I'm going to merge this with the logging issue you mentioned fixed, but if you feel that there are further improvements, please let me know and I can open a follow-up PR to continue iterating on this. |
@WordPress/openverse-maintainers heads up that you may need to run |
Ahh perfect, that's exactly what I was hoping for! Thanks! |
Fixes
Fixes #4256 by @sarayourfriend
Description
As discussed in the linked issue, this PR switches to using the locally available prettier for the pre-commit check.
I've configured it to match how the pre-commit hook worked, including
--ignore-unknown
so that prettier doesn't complain about being passed files it doesn't have a parser for.Incidentally, we no longer need to reproduce the list of prettier dependencies in the pre-commit file, so I really think this is better overall for the maintainability of our prettier usage anyway.
Testing Instructions
CI should pass of course.
Checkout the branch and make changes to markdown files or JavaScript files, any file Prettier should format. Then try to commit those and confirm that the pre-commit hook works as expected: it prevents the commit and writes the changes to disk, just like the old hook did.
Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin