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 official pre-commit hooks instead of poetry executables for pre-commit hooks #705

Closed
CoolCat467 opened this issue Dec 16, 2023 · 9 comments
Labels
meta: wontfix This will not be worked on

Comments

@CoolCat467
Copy link
Contributor

I've run into this issue on my computer as well, and the same issue is now happening on pre-commit.ci: Nothing "forces" per say the computer that is running the pre-commit hooks to have the packages that poetry run is attempting to use be installed. I would suggest switching over to using the hooks officially supplied by the projects we are using, which will also mean we can do things like auto-fixing PRs if the hooks we use have auto-fix support.

@PerchunPak
Copy link
Member

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

@CoolCat467
Copy link
Contributor Author

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.

@ItsDrike
Copy link
Member

ItsDrike commented Dec 16, 2023

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.

@PerchunPak
Copy link
Member

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

@PerchunPak
Copy link
Member

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)

@ItsDrike
Copy link
Member

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

@CoolCat467
Copy link
Contributor Author

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

@ItsDrike
Copy link
Member

ItsDrike commented Dec 16, 2023

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.

@kevinkjt2000 kevinkjt2000 added the meta: wontfix This will not be worked on label Jan 1, 2024
@kevinkjt2000
Copy link
Contributor

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.

@PerchunPak PerchunPak closed this as not planned Won't fix, can't repro, duplicate, stale Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta: wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants