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

Add pre-commit hook to check poetry lock #6450

Closed
wants to merge 5 commits into from

Conversation

nzig
Copy link

@nzig nzig commented Sep 8, 2022

This PR fixes a few things with the repo's pre-commit hooks:

  • files regex - escape ..
  • Add files to poetry-lock, so that it doesn't run on every commit
  • Remove language_version: python3.
  • Add a hook that runs poetry lock --check

Edit: removed features that are already upstream.

mkniewallner
mkniewallner previously approved these changes Sep 18, 2022
mkniewallner
mkniewallner previously approved these changes Sep 22, 2022
@mkniewallner
Copy link
Member

@neersighted I'd like to see that backported to 1.2 branch as well, unless you think that only the hook fixes (and not the new hook) should be backported to 1.2. Any opinion?

@neersighted
Copy link
Member

I'm not 100% sure on this just yet -- or rather I'm not sure on the future of pre-commit hooks in this repo given that upstream is incompatible with our release cycle. But I suppose that should not necessarily stop us from improving the hooks in this repo -- it's just that I hesitate to show more support/commitment for them when they may not be viable in the long run.

@mkniewallner mkniewallner added the status/needs-consensus Consensus among maintainers required label Sep 23, 2022
@mkniewallner mkniewallner dismissed their stale review September 23, 2022 08:43

Need more discussion

@mkniewallner
Copy link
Member

I'm not 100% sure on this just yet -- or rather I'm not sure on the future of pre-commit hooks in this repo given that upstream is incompatible with our release cycle. But I suppose that should not necessarily stop us from improving the hooks in this repo -- it's just that I hesitate to show more support/commitment for them when they may not be viable in the long run.

Makes sense. I've removed my review and added status/needs-consensus label to prevent any accidental merge.

Happy to discuss that further if needed.

@nzig
Copy link
Author

nzig commented Sep 28, 2022

I'm not 100% sure on this just yet -- or rather I'm not sure on the future of pre-commit hooks in this repo given that upstream is incompatible with our release cycle. But I suppose that should not necessarily stop us from improving the hooks in this repo -- it's just that I hesitate to show more support/commitment for them when they may not be viable in the long run.

Could you clarify what you mean by "upstream is incompatible with our release cycle"? You mean that pre-commit hooks are tied to git refs rather than Poetry releases? Or something else I'm misunderstanding?

@finswimmer
Copy link
Member

Could you clarify what you mean by "upstream is incompatible with our release cycle"?

I guess @neersighted is talking about the fact, that pre-commit autoupdate does not work for the pre-commit hooks provided in this repo. See the FAQ about this (https://python-poetry.org/docs/pre-commit-hooks/#why-does-pre-commit-autoupdate-not-update-to-the-latest-version) and the linked issues there.

@neersighted
Copy link
Member

Could you clarify what you mean by "upstream is incompatible with our release cycle"?

I guess @neersighted is talking about the fact, that pre-commit autoupdate does not work for the pre-commit hooks provided in this repo. See the FAQ about this (https://python-poetry.org/docs/pre-commit-hooks/#why-does-pre-commit-autoupdate-not-update-to-the-latest-version) and the linked issues there.

Correct -- we may need to provide pre-commit support through a different repo with a different approach to history.

@nzig
Copy link
Author

nzig commented Sep 28, 2022

I see. Thanks for clarifying.

Prior to this repo having pre-commit hooks defined, I set up https://github.com/medigateio/poetry-pre-commit for doing the same thing internally at my company (also using the 3rd-party poetry-lock-check for getting poetry lock check in 1.1). You could create such a repo with a history like pre-commit expects to get around that problem. I'd be happy to help!

@nzig
Copy link
Author

nzig commented Jul 9, 2024

Hey @neersighted , what are your thoughts on merging this?
It seems the current .pre-commit-hooks.yaml continues to be maintained despite no solution for pre-commit autoupdate, so maybe we could merge this?

I'm also happy to help with setting up a separate repo for the pre-commit hooks to fix the autoupdate issue, if you'd like.

@nzig nzig changed the title Various fixes to pre-commit hooks Add pre-commit hook to check poetry lock Jul 9, 2024
@nzig
Copy link
Author

nzig commented Jul 9, 2024

Actually, I see that maybe all of the features of this PR were added upstream. The only thing that remains is a hook that runs pre-commit check --lock, but you already have one that runs pre-commit check so it's probably not needed.

I'll close this PR.

@nzig nzig closed this Jul 9, 2024
Copy link

github-actions bot commented Aug 9, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status/needs-consensus Consensus among maintainers required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants