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.

Sign up for GitHub

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

Clean up check.sh, move stuff to pre-commit #3157

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+25 −42
Open

Clean up check.sh, move stuff to pre-commit #3157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b3783f
remove ruff, black, codespell, and gen-exports from check.sh as they'…
jakkdl Dec 17, 2024
84433f4
move pip-compile tests to pre-commit
jakkdl Dec 17, 2024
7557558
restore pip-compile in check.sh, skip in CI
jakkdl Dec 17, 2024
b38faba
make regenerate-files runnable in CI, with the downside of not freezi…
jakkdl Dec 17, 2024
17d3d05
fix skip
jakkdl Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ ci:
autofix_prs: true
autoupdate_schedule: weekly
submodules: false
skip: [regenerate-files]
skip: ["pip-compile"]

repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
Expand Down Expand Up @@ -40,7 +40,30 @@ repos:
hooks:
- id: regenerate-files
name: regenerate generated files
language: system
language: python
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that strings are quoted randomly in some places and not others. It's rather inconsistent. Perhaps, just don't quote them unnecessarily?

Copy link
Member Author

@jakkdl jakkdl Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

that's just me being bad at yaml and not knowing what's necessary or not :p
perhaps time for a yaml-formatter, but I'd probably lean towards unnecessary-quotes over relying on (python) people knowing the quoting rules of yaml

Copy link
Member

Choose a reason for hiding this comment

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

Well, then you'd have to wrap literally everything in the file, including the keys:

Suggested change
language: python
"language": "python"

Numbers like 3.11 are cast to float otherwise. And 3.10 caught many people by surprise back in the day (it's actually 3.1 unless quoted explicitly).

FWIW, I don't really like quotes. But if something needs escaping, I tend to use the following syntax (especially if there are quotes in the value that I don't want to bother about):

Suggested change
language: python
language: >-
python

entry: python src/trio/_tools/gen_exports.py
pass_filenames: false
additional_dependencies: ["astor", "attrs", "black", "ruff"]
files: ^src\/trio\/_core\/(_run|(_i(o_(common|epoll|kqueue|windows)|nstrumentation)))\.py$
- repo: https://github.com/astral-sh/uv-pre-commit
rev: 0.5.9
hooks:
# Compile requirements
- id: pip-compile
name: uv pip-compile test-requirements.in
args: [
"--universal",
"--python-version=3.9",
"test-requirements.in",
"-o",
"test-requirements.txt"]
Comment on lines +54 to +59
Copy link
Member

Choose a reason for hiding this comment

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

I think this reads better:

Suggested change
args: [
"--universal",
"--python-version=3.9",
"test-requirements.in",
"-o",
"test-requirements.txt"]
args:
- --universal
- --python-version=3.9
- --output-file=test-requirements.txt
- test-requirements.in

Copy link
Member

Choose a reason for hiding this comment

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

That does look a bit nicer, I agree

Copy link
Member Author

Choose a reason for hiding this comment

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

in a python repo I'd argue that using python-looking syntax is going to be easier for people to understand.

Copy link
Member

Choose a reason for hiding this comment

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

That is a pretty good argument actually

Copy link
Member

Choose a reason for hiding this comment

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

Dunno, it's usually harder for me as it looks like type annotations, but it isn't.

files: ^test-requirements\.(in|txt)$
- id: pip-compile
name: uv pip-compile docs-requirements.in
args: [
"--universal",
"--python-version=3.11",
"docs-requirements.in",
"-o",
"docs-requirements.txt"]
files: ^docs-requirements\.(in|txt)$
40 changes: 0 additions & 40 deletions check.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,44 +11,6 @@ if [ -z "${GITHUB_STEP_SUMMARY+x}" ]; then
ON_GITHUB_CI=false
fi

# Test if the generated code is still up to date
echo "::group::Generate Exports"
python ./src/trio/_tools/gen_exports.py --test \
|| EXIT_STATUS=$?
echo "::endgroup::"
Copy link
Contributor

Choose a reason for hiding this comment

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

I probably should do this in another PR but I think we should move gen exports out of pre-commit and back here. Unfortunately, as things currently are, if you modify a file that the pre-commit hook says it uses then commit with a git client (ie not just git commit -m "..." in the venv you have for trio), gen_exports doesn't get the dependencies it needs.

So far I've been getting that error then going to my terminal to run git there, but that's a bit much.

Copy link
Member

Choose a reason for hiding this comment

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

Pre-commit with a external git client doesn't install required dependencies?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, because we don't specify them. We don't specify them because pre-commit doesn't let you install from a requirements file :(

Copy link
Member Author

Choose a reason for hiding this comment

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

I specified them in the latest commit:

trio/.pre-commit-config.yaml

Line 46 in 17d3d05

additional_dependencies: ["astor", "attrs", "black", "ruff"]

but that doesn't let us pin the versions (in a non-awful way), so yeah if we don't want that then it has to go outside of pre-commit

Copy link
Member

Choose a reason for hiding this comment

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

trio/.pre-commit-config.yaml

Line 46 in 17d3d05

additional_dependencies: ["astor", "attrs", "black", "ruff"]

Keep in mind that the pre-commit cache is virtually immortal. You can clean it locally, but not in the pre-commit.ci service. For that, changing the deps list or the hook version is required. This may be problematic sometimes.

So I've found that the best thing to do here is to keep the version pins in the list and bump them periodically. Just so that the cache doesn't grow too old.


# Autoformatter *first*, to avoid double-reporting errors
# (we'd like to run further autoformatters but *after* merging;
# see https://forum.bors.tech/t/pre-test-and-pre-merge-hooks/322)
# autoflake --recursive --in-place .
# pyupgrade --py3-plus $(find . -name "*.py")
echo "::group::Black"
if ! black --check src/trio; then
echo "* Black found issues" >> "$GITHUB_STEP_SUMMARY"
EXIT_STATUS=1
black --diff src/trio
echo "::endgroup::"
echo "::error:: Black found issues"
else
echo "::endgroup::"
fi

# Run ruff, configured in pyproject.toml
echo "::group::Ruff"
if ! ruff check .; then
echo "* ruff found issues." >> "$GITHUB_STEP_SUMMARY"
EXIT_STATUS=1
if $ON_GITHUB_CI; then
ruff check --output-format github --diff .
else
ruff check --diff .
fi
echo "::endgroup::"
echo "::error:: ruff found issues"
else
echo "::endgroup::"
fi

# Run mypy on all supported platforms
# MYPY is set if any of them fail.
MYPY=0
Expand Down Expand Up @@ -94,8 +56,6 @@ if git status --porcelain | grep -q "requirements.txt"; then
echo "::endgroup::"
fi

codespell || EXIT_STATUS=$?
Copy link
Contributor

@A5rocks A5rocks Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(not the right line because github isn't letting me comment there)

Should you remove the uv pip compile lines above? And also update the error message + add a pre-commit run -a line in check.sh so people can run check.sh locally? (I'm not sure people do run check.sh locally but...)

Copy link
Member Author

@jakkdl jakkdl Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Realized we can't do uv pip compile in pre-commit in CI since it needs internet, so it needs to be here (or somewhere).

My goal is not to require/expect people to run check.sh locally as it's unintuitive and clunky, but given the limitations of pre-commit we do need to stash these somewhere and make it easy for users to run them locally if they want.
I'd personally vote for tox (due to familiarity), and set up environments for the different tests.


echo "::group::Pyright interface tests"
python src/trio/_tests/check_type_completeness.py || EXIT_STATUS=$?

Expand Down
Loading