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

Prettier pre-commit has been archived #1880

Closed
trallard opened this issue Jun 12, 2024 · 8 comments
Closed

Prettier pre-commit has been archived #1880

trallard opened this issue Jun 12, 2024 · 8 comments
Labels
needs: discussion Needs discussion before an implementation can be made

Comments

@trallard
Copy link
Collaborator

trallard commented Jun 12, 2024

I have been banging my head because I changed computers and thought I had broken stuff on my end.
After debugging my setup, I noticed that for whatever reason, if I called pre-commit run --all-files or tried to run pre-commit as a git hook, it kept erroring, but calling it through tox and pre-commit ci seemed to work still.

I then realised that the prettier pre-commit mirror had been archived due to prettier breaking a lot of plugins (see https://github.com/pre-commit/mirrors-prettier/blob/main/README.md), so I assume I just hit a fresher install of dependencies which broke my prettier hook.

Considering this, it might be worth removing the hook from our pre-commit config as it is just a matter of time before this breaks in other places. I assume the alternative would be to use prettier as a husky hook but not sure if that would be too much hassle.

@trallard trallard added the needs: discussion Needs discussion before an implementation can be made label Jun 12, 2024
@drammock
Copy link
Collaborator

I noticed last week that the mirror had been archived, when investigating the CI failures in #1849 and #1850 but I didn't connect the dots. (Meanwhile to unblock CIs we did #1856 instead).

In the course of that I did get the husky thing set up locally (at least to the point where it ran on commit) but it did seem like a hassle... I don't recall the exact details but it generally seemed to be organized/controlled differently than other hooks I've encountered.

Other things we could try instead of Prettier are https://eslint.org/ and https://standardjs.com/. Looks like StandardJS is zero config (no decisions to make) and has an easy pre-commit setup: https://standardjs.com/#use-a-pre-commit-hook so I'd vote to use that one

@12rambau
Copy link
Collaborator

But the thing is that prettier was covering more than just JS, it was linting every non python files if I'm correct so it needs to be replaced by more than 1 component if we want to keep the same level of coverage.

@trallard
Copy link
Collaborator Author

Yep prettier was looking at more than JS so it's a bit of a pain finding multiple replacements.

Husky hooks do feel and install differently than pre-commit hooks since they are based on/intended for JS projects and workflows. What worries me about adding this as a husky hook is the extra complexity for contributors

It might be worth looking again at a bunch of pre-commit hooks we can use to cover the prettier cases and only consider husky ones as a last resource

@drammock
Copy link
Collaborator

It might be worth looking again at a bunch of pre-commit hooks we can use to cover the prettier cases and only consider husky ones as a last resource

+1. I'd be willing to switch from prettier to StandardJS right away and fill in the other file types in subsequent PRs. But if y'all prefer to find replacements first, that's OK too.

Here's a start:

@12rambau
Copy link
Collaborator

12rambau commented Jun 13, 2024

what about the pre-commit language specific hooks: https://github.com/macisamuele/language-formatters-pre-commit-hooks ?

@12rambau 12rambau reopened this Jun 13, 2024
@drammock
Copy link
Collaborator

what about the pre-commit language specific hooks: https://github.com/macisamuele/language-formatters-pre-commit-hooks ?

Of the languages it supports I think the only one we use is toml.

Here's a couple more:

@trallard
Copy link
Collaborator Author

Ok I can look into re-adding some of this next week

@drammock
Copy link
Collaborator

drammock commented Dec 5, 2024

I think this was solved by #2049 (which replaced the archived prettier pre-commit with a fork managed by pycontribs).

@drammock drammock closed this as completed Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: discussion Needs discussion before an implementation can be made
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants