pydata / pydata-sphinx-theme

A clean, three-column Sphinx theme with Bootstrap for the PyData community
https://pydata-sphinx-theme.readthedocs.io
BSD 3-Clause "New" or "Revised" License
597 stars 312 forks source link

Prettier pre-commit has been archived #1880

Open trallard opened 3 months ago

trallard commented 3 months ago

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.

drammock commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

drammock commented 3 months ago

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 commented 3 months ago

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