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

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

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

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Dec 17, 2024

Ruff, Black, Codespell and gen-exports are all run in pre-commit, so there's no need to also run them in check.sh
moving uv pip-compile to pre-commit was easy

The only remaining tools in check.sh now are:

  1. pyright pyright+pre-commit+CI does not play well, as pyright ~requires an internet connection and pre-commit in CI isolates the environment not to have it. In flake8-async we have a dedicated CI action for pyright that uses an unofficial github action.
    EDIT: Though we also have check_type_completeness.py that won't play well with that pyright-action.

  2. mypy I think doing 3 full runs (all platforms) would be overly slow for local development, but I'm not sure it's possible to specify manual stages to run with the pre-commit github action. So best solution here is perhaps to configure a mypy pre-commit hook (using current system?) and once again add a dedicated CI action.

Addresses some of #2699

@jakkdl jakkdl requested a review from CoolCat467 December 17, 2024 11:51
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.62%. Comparing base (c4c8ce4) to head (17d3d05).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3157   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files         122      122           
  Lines       18405    18405           
  Branches     1226     1226           
=======================================
  Hits        18336    18336           
  Misses         47       47           
  Partials       22       22           

@jakkdl
Copy link
Member Author

jakkdl commented Dec 17, 2024

ah right, of course pip-compile also requires internet. There's maybe a configuration with repo: local and language: system but eh. Unclear if it's worth running locally

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 :(

@@ -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

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

@CoolCat467 CoolCat467 left a comment

Choose a reason for hiding this comment

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

Moving things to pre-commit looks great, just wondering about how this will work with CI. Will CI check to make sure pre-commit run succeeds before allowing merge, or do we have to change what is required in the "all greens" workflow?

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