8000 feat: Update pre-commit mirror script by sudosubin · Pull Request #53 · astral-sh/ruff-pre-commit · GitHub
[go: up one dir, main page]

Skip to content

feat: Update pre-commit mirror script #53

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

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

sudosubin
Copy link
Contributor

Summary

  • pre-commit-mirror-maker is not suitable for generating multiple hooks in single .pre-commit-hooks.yaml file.
  • So replaced the script and github workflows.

Test Plan

  • Update pyproject.toml's ruff==0.0.290 into ruff==0.0.289, and run the script.
$ python3 mirror.py
...

@charliermarsh
Copy link
Member

Nice, thank you so much! We'd just discussed doing this internally, you read our minds... I will defer to @zanieb for review. (Zanie's out on vacation so will probably get to this next week.)

Copy link
Member
@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! I was just complaining about the mirror not working with the new hook :) I have a few questions.


- uses: actions/setup-python@v4
with:
python-version: "3.x"

- run: pip install pre-commit-mirror-maker
- run: pip install "urllib3>=2" tomli packaging
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should add a requirements-dev.txt for these with pinned versions? e.g. we use pip-compile over in ruff-lsp (https://github.com/astral-sh/ruff-lsp).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I listed dependencies through optional-dependencies in pyproject.toml and create a requirements-dev.txt file through pip-tools.

The setup-python action now installs the packages via the generated requirements-dev.txt file and runs the mirror.py script.

mirror.py Outdated
import typing
from pathlib import Path

import tomli
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified the setup-python action usage, and replaced to use built-in tomllib module.

mirror.py Outdated
def get_current_version(pyproject: dict) -> Version:
requirements = [Requirement(d) for d in pyproject["project"]["dependencies"]]
requirement = next((r for r in requirements if r.name == "ruff"), None)
assert requirement is not None
Copy link
Member

Choose a reason for hiding this comment

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

Should we include an assertion message that would help us diagnose this failed assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some assertion messages, but not sure if I need to supplement.

Comment on lines -16 to -18
# Only commit on change.
# https://stackoverflow.com/a/9393642/3549270
if check_output(["git", "status", "-s"]).strip():
Copy link
Member

Choose a reason for hiding this comment

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

Is this behavior retained?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I updated to retain this behavior.

Copy link
Member
@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Thanks! This looks great

@zanieb zanieb merged commit ba4269e into astral-sh:main Sep 26, 2023
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
0