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

fix: use uv tool run for pre-commit if not global #300

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lengau
Copy link
Contributor

@lengau lengau commented Dec 13, 2024

Issue found here: canonical/craft-store#241 (review)

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

@medubelko
Copy link
Contributor

I'm not entirely sure what the intended result for pre-commit is, but I think my host env is too dirtied at this point to replicate the original conditions. I:

  • Ran make clean.
  • Removed uv.
  • Removed all references to uv from my bash scripts.
  • Removed pre-commit from ~/.local/bin/.
  • Removed pre-commit from .git/hooks/

When I ran which pre-commit, it yielded nothing.

Then, when I re-ran make setup, it replied with:

sudo snap install --classic astral-uv
astral-uv 0.5.7 from Alex Lowe (lengau) installed
uv tool install pre-commit
uv tool run pre-commit install
`pre-commit` is already installed
pre-commit installed at .git/hooks/pre-commit
uv sync --frozen --all-extras

Is the bit about "already installed" wrong? Should it be installing as a script in .git/hooks/? I thought it was installing a binary.

@lengau
Copy link
Contributor Author

lengau commented Dec 17, 2024

@medubelko pre-commit I think stores things elsewhere - try running pre-commit uninstall?

@medubelko
Copy link
Contributor

pre-commit is completely absent from my system at this point, which is why I'm confused about why the log says I have it.

@lengau
Copy link
Contributor Author

lengau commented Dec 19, 2024

uv still had pre-commit installed as a tool, so I think it's fine.

@lengau lengau requested a review from a team December 19, 2024 22:05
Copy link
Contributor

@dariuszd21 dariuszd21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

common.mk Outdated Show resolved Hide resolved
Copy link
Contributor

@medubelko medubelko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but I'm not confident in my own testing of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants