Use official pre-commit hooks instead of poetry executables for pre-commit hooks about mcstatus HOT 9 CLOSED

I'm strongly opposed to this.

I've used system commands very deliberately when I added pre-commit to this project. That's because not only do some linters not have official pre-commit hooks provided (even though that might no longer be the case for those which we're using now, it might become relevant the moment we try to move to something else, and I don't want to have to have some local hooks, and other CI hooks), but even the tools which do provide these often don't do a great job at keeping them up-to-date with the latest actual release of that tool.

The version of the individual tools used by pre-commit locally are all listed in pyproject.toml, and it is your responsibility to run poetry install to install them locally, and to update the dependencies as pyproject.toml changes. This is the case for any project dependencies, not just those used in pre-commit.

It is not enough to simply declare the dependencies in pre-commit, and to omit these tools from pyproject.toml entirely, as we need the actual executables to be made available in the project's virtual environment. This is because many editors pick up on that, and then use the proper versions of these linters, as installed in the environment.

I'm also against the option to have the versions listed both in pre-commit, and in pyproject.toml, as we would need to keep those versions consistent, and pay great attention that this remains the case as updates come out.

In my opinion, there is no reason to oppose using locally installed linters, considering they're properly declared and will be installed with poetry install, and if you just have an issue with running poetry install every time the pyproject.toml is updated, perhaps then it may be worth it to create a script that runs as a part of pre-commit (another local pre-commit hook), and checks that your version of these tools matches the one declared in pyproject, though I'm not sure how hard of a task this would be. Alternatively, we could even add poetry install as a command to just run as a part of pre-commit, before any of the other linters run.

from mcstatus.

Revoked pre-commit's access to this repo until this issue will be fixed/resolved (I mean, there is no sense to keep CI turned on if it always fails)

from mcstatus.

Doesn't it mean that we have to install one dependency twice?

from mcstatus.

Not if you only run pre-commit, no. If you run the full CI locally, then yes, this would be 'installing it twice', and there's no real way to access pre-commit's virtual environments manually in a nice fashion I'm pretty sure. I think it would be worth it though, because I don't like the idea of what's running being system dependent. I would much rather have pre-commit handle installing the correct version of everything than having the system have the right version of everything, if that makes any sense.

from mcstatus.

And by the way, we already run pre-commit in Validation workflow

though I'm not sure how hard of a task this would be

https://python-poetry.org/docs/master/pre-commit-hooks/#poetry-install

from mcstatus.

https://python-poetry.org/docs/master/pre-commit-hooks/#poetry-install

Oh, that's really nice! This would be a skipped hook in the GitHub CI, as we install with ItsDrike/setup-poetry action, but for local updating, I think it might actually be worth doing. poetry install is a no-op if there were no updates anyway, so it's pretty much perfect.

What I do wonder about here is whether using the official hook would also install poetry as a tool, it it's not found. I very much doubt it, but if it would, we could actually use that to fix pre-commit.ci

from mcstatus.

You can add a skip=[<hook id>,<hook id>] in in the ci section if required

from mcstatus.

You can add a skip=[<hook id>,<hook id>] in in the ci section if required

Can we just skip all of the local hooks then? This would probably solve the issue of it failing, without having to change to the remote hooks, no?

So, unless there's a reason this wouldn't work, or a really good reason to convince me against local hooks that addresses the points I made earlier, I'd like to see this solution instead.

from mcstatus.

Pre-commit.ci was briefly adopted to update tag versions in pre-commit-config.yaml. This is something that pre-commit autoupdate can do. This issue emerged as part of adopting pre-commit.ci, which was already abandoned as an effort. Moving to close this.

from mcstatus.