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 config #88

Merged
merged 1 commit into from
Jan 11, 2023
Merged

add pre-commit config #88

merged 1 commit into from
Jan 11, 2023

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Dec 19, 2022

It bit me a bunch of times that I didn't run tox before pushing minor changes, and tox -e check takes 13s on my system, so I've gradually recreated all the functionality from it in pre-commit + some other checks. So at this point it might be worth removing tox -e check entirely and transitioning the CI to use pre-commit as well. If you approve of the idea I'll implement the necessary gh CI changes as well & clean up tox.ini.

(though it currently won't pass until #87 is merged)

TODO:

@Zac-HD
Copy link
Member

Zac-HD commented Dec 19, 2022

Sounds good to me; mark ready-to-review when... you know... and I'll be happy to merge.

@jakkdl jakkdl self-assigned this Dec 20, 2022
@jakkdl jakkdl force-pushed the pre-commit branch 6 times, most recently from f899845 to 6d23bd7 Compare January 1, 2023 16:05
@jakkdl jakkdl marked this pull request as ready for review January 1, 2023 16:12
@jakkdl
Copy link
Member Author

jakkdl commented Jan 1, 2023

I added https://pre-commit.ci/ to my fork, and it now passes on my mirror PR over there: https://github.com/jakkdl/flake8-trio/pull/1

The largest issue was that pyright requires internet connectivity to run, which the pre-commit ci does not support

There's a couple options to get around that:

  • Don't run pyright in pre-commit, but have a tox action for it. I'd prefer not to do this, since I want local pre-commit hooks to still run pyright when committing.
  • [current solution] skip pyright when run in ci, but have a separate tox environment that runs pre-commit run pyright --all-files. This environment is not included in the default envlist.
    • One downside is that pre-commit will still initialize all hook environments, even if only a single hook is specified, so there's a slight time penalty here.
    • This will necessarily have to check all files (unless the modified file list is piped from the gh ci action into tox somehow) so will be slower - but this is probably an upside since an unmodified file can depend on a type specified in a modified file.
  • Use https://pre-commit.ci/lite.html instead, so pre-commit is run as a GH action. Then it will have internet connectivity, but losing out on speed and automatic PR's that update hooks.
  • Use mypy instead of pyright 😉 (idk why you went with pyright from the beginning actually, but since I've had mypy enabled in my editor since previously it runs perfectly fine in mypy as well - though not with --strict (but most of those are function signatures not having -> None which pyright doesn't require so it's been inconsistently applied)

Various other considerations for this, or future PR's, or never:

  • Run minimal tox from pre-commit, so developers (me) don't accidentally push code with broken tests. Don't think it happens enough to bother figuring out the piping.
  • Don't run tests with coverage on any platform but py311-flake8_6. Running with coverage slows down considerably, and means extra # noqa's (I've hacked together some ugly aliases that mostly solves this locally)
  • flake8-eradicate still doesn't work with flake8-6, and I haven't found a way to tell pre-commit to exclude specific hook id's from autoupdating, so with the full CI service we'll likely have it nagging us that flake8 is out of date.
  • Do we want to disable autofix_prs? I haven't used pre-commit ci enough to know how handy vs irritating vs dangerous it is, but noticed that flake8-bugbear has it disabled.
  • flake8-bandit prints weird error messages for me: Unable to find qualified name for module PyCQA/bandit#725 and with me doing a bunch of illegal stuff in test-flake8-trio it might just make sense to ditch it

But otherwise I think this should be good for merging

@jakkdl
Copy link
Member Author

jakkdl commented Jan 9, 2023

rebased on top of main

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Two small actionable comments, then ready to merge!

flake8_trio.py Outdated Show resolved Hide resolved
Comment on lines +13 to +16
hooks:
- id: pyright
entry: env PYRIGHT_PYTHON_FORCE_VERSION=latest pyright
args: ['--pythonversion=3.11', '--warnings']
Copy link
Member

Choose a reason for hiding this comment

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

I forget the exact syntax here, but we'll want this to run on all-files rather than passing changed filenames, because changing one file can create typecheck errors in another unchanged 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.

In the CI check it will run on all files (it runs pre-commit run pyright --all-files), but I personally prefer the commit hook not to run on all files as that comes with a decently hefty time penalty (5->10 seconds on my machine).

Comment on lines +43 to +58
- flake8-builtins
- flake8-bugbear
- flake8-comprehensions
- flake8-2020
- flake8-bandit
- flake8-builtins
- flake8-bugbear
- flake8-comprehensions
- flake8-datetimez
#- flake8-docstrings
- flake8-mutable
- flake8-noqa
- flake8-pie
- flake8-pytest-style
- flake8-return
- flake8-simplify
Copy link
Member

Choose a reason for hiding this comment

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

🚀

(makes my pin-your-deps senses tingle, but no worse than the status quo I suppose... I really want better transitive-pins support from pre-commit but it's a pretty big project)

@jakkdl jakkdl force-pushed the pre-commit branch 3 times, most recently from 3ced8cc to 98938ef Compare January 10, 2023 14:48
several flake8 plugins
pyright is not run with the pre-commit ci, since it requires an internet
connection and the free tier for FOSS projects does not allow it. So
it's called by tox in a separate GitHub action
also fixes several minor style violations reported by those plugins, and
adding a few comments
@jakkdl
Copy link
Member Author

jakkdl commented Jan 10, 2023

Renamed the check ci action to show that it's now solely for type-checking. I tried doing that previously but failed, but idk what I did different that time.

@Zac-HD Zac-HD merged commit cd59da3 into python-trio:main Jan 11, 2023
@jakkdl jakkdl mentioned this pull request Jan 11, 2023
@jakkdl jakkdl deleted the pre-commit branch January 11, 2023 13:40
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.

2 participants