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 with ruff #42

Merged
merged 3 commits into from
Jan 20, 2025

Conversation

ShaiahWren
Copy link
Contributor

@ShaiahWren ShaiahWren commented Jan 17, 2025

[AAP-38768]

Description

This work ensures that each time a developer attempts a commit,

  • a pre-commit hook with uv+ruff for linting and formatting is run
  • in order to arrest the commit if linting/formatting discrepancies are detected and provide useful feedback in the output log.

Set-up

  • in /metrics-utility root dir, run uv sync & uv run ruff / uvx ruff to manage the uv virtual env and add the dependencies.
  • Ensure deps have been installed with checks such as ruff --version; pytest --version; django-admin --version; pre-commit --version;
  • If deps are missing try uv sync again

Testing

  • create a small test file, such as "test.py", and type any incorrectly formatted or linted python.
  • add the file and attempt to commit the change

Expected Results

Commit should not complete; it will direct you some output that explains what the pre-commit-with-uv+ruff found.

Copy link

@dhaustein dhaustein left a comment

Choose a reason for hiding this comment

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

I failed to make it work on my end. I can do $ pre-commit run manually and that will pickup the errors I made in my test.py file.

$ pre-commit run
ruff.....................................................................Failed
- hook id: ruff
- exit code: 1

But when I do git add test.py and git commit -m "commit bad file" it will happily let me.

$ vim test.py  # added the example from here https://realpython.com/ruff-python/#checking-for-errors

$ git add .

$ git commit -m 'test: test pre-commit hook'
[pr/42 59ac220] test: test pre-commit hook
 1 file changed, 18 insertions(+)
 create mode 100644 test.py

$ git status
On branch pr/42
nothing to commit, working tree clean

🎆 EDIT: I had to re-run $ pre-commit install and then everything started working as expected. I suggest adding this as an extra install step to the Set-up steps.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@art-tapin art-tapin self-requested a review January 20, 2025 12:45
@ShaiahWren ShaiahWren force-pushed the pre-commit-with-uv+ruff branch from c8e0ab8 to 0a21af6 Compare January 20, 2025 16:00
@himdel himdel mentioned this pull request Jan 20, 2025
6 tasks
@himdel
Copy link
Collaborator

himdel commented Jan 20, 2025

The changes look good 👍 ,

just, did you run uv sync after the last change? Looks like when I git pr 42; uv sync, it updates the lockfile, adding pre-commit, so maybe it wasn't up to date?

(Also, it warns about a missing requires-python, adding a requires-python = ">=3.12" to the project section in pyproject.toml, and a doing an echo 3.12 > .python-version seems to fix this .. unless we're keeping the version out deliberately?)

@himdel
Copy link
Collaborator

himdel commented Jan 20, 2025

In this PR description's Set-up section:

in /metrics-utility root dir, create and activate your venv with python -m venv venv; source venv/bin/activate
install the deps using pip install

This bit seems surprising? uv manages virtual environmeents & dependencies for you, so I'd expect these to be something like "run uv sync" & uv run ruff / uvx ruff etc.? Or uv venv ; source .venv/bin/activate instead of directly talking to virtualenv & pip?

@ShaiahWren ShaiahWren force-pushed the pre-commit-with-uv+ruff branch from 0a21af6 to 27411ae Compare January 20, 2025 18:35
Copy link
Collaborator

@himdel himdel left a comment

Choose a reason for hiding this comment

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

LGTM 👍 , no warnings, no diff after sync, works for me :)

@ShaiahWren ShaiahWren merged commit c8c5556 into ansible:devel Jan 20, 2025
2 checks passed
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