Add pre-commit #2744

CoolCat467 · 2023-08-07T02:10:50Z

Up for debate, but I find pre-commit auto-formatting code for my own projects very useful, and thought I would suggest trio adopting it as well.

Basically pre-commit uses git's hooks system to do anything you want it too, all defined from one file. They have more information about it on their website (https://pre-commit.com/).
They also have a continuous integration system (more info at https://pre-commit.ci/) that means you can do things like automatically doing things like code formatting on pull requests and committing directly.

This PR adds the config file pre-commit looks for, and has pre-commit do black code formatting and isort, as well as flake8, but I'm not super sure about that because sometime I plan to make another PR for moving over to use ruff instead, so it's skipped in the ci run section currently.

codecov · 2023-08-07T02:13:08Z

Codecov Report

Merging #2744 (9978f6e) into master (4c38ba9) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2744   +/-   ##
=======================================
  Coverage   98.92%   98.92%           
=======================================
  Files         113      113           
  Lines       16728    16728           
  Branches     3026     3026           
=======================================
  Hits        16548    16548           
  Misses        124      124           
  Partials       56       56

A5rocks · 2023-08-08T03:31:39Z

.pre-commit-config.yaml

+ci:
+  autofix_commit_msg: "[pre-commit.ci] auto fixes from pre-commit.com hooks"
+  autofix_prs: true
+  autoupdate_commit_msg: "[pre-commit.ci] pre-commit autoupdate"
+  autoupdate_schedule: weekly
+  skip: [flake8]
+  submodules: false


I don't think we want pre-commit CI (other than maybe for updating the pre-commit hooks). We're intentionally not pushing commits based on what autoformatters want since that might interfere with an WIP PR, for instance.

Running Black on WIP PR's should not be a problem. isort could in theory be a problem, but I suspect it'd help more often than it hurts - and in any case it doesn't modify your local repo and you can review the commit before isort does its thing if you want.

But it can definitely be messy, it's probably a tradeoff between experienced contributors (that will have taken the decision whether to run the pre-commit hooks locally) vs new contributors (that might struggle with setting up tooling and will waste time doing several CI runs until stuff passes) and people maybe having to review unformatted code.

case in point 😅

Yeah I keep using the web editor for small changes :(

I do think for larger PRs changing the code from under their feet is a bit annoying.

using the web editor seems like a strong case in favor of it. Leaving it off at the beginning seems reasonable regardless just to make sure it works and all, and then we can try toggling it later

jakkdl

Yeah I've been thinking about this in Use tox and/or pre-commit for static analysis checks #2699 and created a .pre-commit-config.yaml for personal use myself.
pre-commit run --all-files will trigger a lot of changes in the black and isort hooks on files not currently checked by ci.sh. Either ignore these or reformat them (in this or another PR). Flake8 also complains about a couple files
You're missing codespell
We probably want mypy in this once one can run mypy . without triggering errors. Running mypy -m trio -m trio.testing --platform=x.y.z should be possible, but clunky and slow
If the long-term plan is to replace ci.sh (which I'm strongly in favor of) you also want the ability to run
- _tools/gen_exports.py
- _tests/_check_type_completeness.py or at least pyright --verifytypes (on all platforms) once that passes
- pip-compile -q ... ... except good luck doing that without tox to manage python version, but there's discussion in python version for CI formatting check #2681 and there's Rework packaging to use Poetry instead. #2595
- Several of these should probably be default-off though
I personally also have autoflake enabled
I've had https://github.com/macisamuele/language-formatters-pre-commit-hooks --pretty-format-toml enabled for this repo, but dropped it at some point for some reason.

.pre-commit-config.yaml

jakkdl · 2023-08-08T11:46:24Z

.pre-commit-config.yaml

+ci:
+  autofix_commit_msg: "[pre-commit.ci] auto fixes from pre-commit.com hooks"
+  autofix_prs: true
+  autoupdate_commit_msg: "[pre-commit.ci] pre-commit autoupdate"
+  autoupdate_schedule: weekly
+  skip: [flake8]
+  submodules: false


Running Black on WIP PR's should not be a problem. isort could in theory be a problem, but I suspect it'd help more often than it hurts - and in any case it doesn't modify your local repo and you can review the commit before isort does its thing if you want.

But it can definitely be messy, it's probably a tradeoff between experienced contributors (that will have taken the decision whether to run the pre-commit hooks locally) vs new contributors (that might struggle with setting up tooling and will waste time doing several CI runs until stuff passes) and people maybe having to review unformatted code.

A5rocks · 2023-08-08T16:45:01Z

FYI mypy shouldn't be used as a pre-commit hook. (The whole point of pre-commit hooks is to run checks on some subset of files and yet mypy should really check all files)

CoolCat467 · 2023-08-08T16:58:36Z

Personally I think the best thing to do is have pre-commit do auto-fixes and actual github actions do the tests that don't have auto-fixes or things that shouldn't be auto-fixed

jakkdl · 2023-08-09T16:47:56Z

FYI mypy shouldn't be used as a pre-commit hook. (The whole point of pre-commit hooks is to run checks on some subset of files and yet mypy should really check all files)

I personally very much like running mypy locally as a pre-commit hook to catch small type errors before I push, and only running it on a subset of files speeds it up so so much. But of course CI should check all files to catch errors introduced in other files

jakkdl · 2023-08-09T16:54:29Z

Personally I think the best thing to do is have pre-commit do auto-fixes and actual github actions do the tests that don't have auto-fixes or things that shouldn't be auto-fixed

hm, I personally like it for any check that's quick to save you having to wait for a failing CI. Esp as pre-commit will limit the subset of files it checks so it's much much faster than running them manually locally as well unless you set up something fancy. Esp something like codespell I see no reason at all not to put in pre-commit

I also think there's a strong case for having all checks configured and run from the same place, parsing the output of pre-commit is much better than digging into the text log of check.sh. You could set checks to be default-off unless in CI for stuff that's slow or otherwise.

jakkdl · 2023-08-09T16:56:54Z

For reference, this is the config I've crafted for flake8-trio: https://github.com/Zac-HD/flake8-trio/blob/main/.pre-commit-config.yaml (most of the flake8 checks are redundant since the addition of ruff though - but I've been keeping them just to see if they ever disagree)

A5rocks · 2023-08-10T06:08:29Z

FYI mypy shouldn't be used as a pre-commit hook. (The whole point of pre-commit hooks is to run checks on some subset of files and yet mypy should really check all files)

I personally very much like running mypy locally as a pre-commit hook to catch small type errors before I push, and only running it on a subset of files speeds it up so so much. But of course CI should check all files to catch errors introduced in other files

As you've noted elsewhere (w/r/t mypy trio vs mypy -m trio) the files you pass and how you pass them is important: running mypy specifically over certain files means some errors might not show up and that others might appear when they don't actually exist. (If it means anything, the stance of mypy's core devs is that pre-commit doesn't work great for mypy)

jakkdl · 2023-08-10T11:46:31Z

FYI mypy shouldn't be used as a pre-commit hook. (The whole point of pre-commit hooks is to run checks on some subset of files and yet mypy should really check all files)

I personally very much like running mypy locally as a pre-commit hook to catch small type errors before I push, and only running it on a subset of files speeds it up so so much. But of course CI should check all files to catch errors introduced in other files

As you've noted elsewhere (w/r/t mypy trio vs mypy -m trio) the files you pass and how you pass them is important: running mypy specifically over certain files means some errors might not show up and that others might appear when they don't actually exist. (If it means anything, the stance of mypy's core devs is that pre-commit doesn't work great for mypy)

The only issue I've hit with false-positives so far is on files that were conditionally imported on different platforms, but that was easily resolved with adding platform type guards to the files. This wasn't isolated to pre-commit though, it also happened with mypy trio (which we might want to switch to in order to include tests and everything) and if you had your IDE configured to run type-checking (which is very common nowadays, so is a very strong argument IMO. My setup were blasting errors at me any time I had those files open). Not finding errors is of course abundant, but CI running mypy on all files will check for that.

I understand mypy not wanting to official support it (python/mypy#13916), but they do link the pre-commit mirror directly in their readme: https://github.com/python/mypy#integrations

.pre-commit-config.yaml

jakkdl

Mostly just the problems with running pre-commit run --all-files that remains a stopper.

CoolCat467 · 2023-08-10T16:27:51Z

I think I'll make another PR running it on all files and fixing anything that happens once this one is merged

jakkdl · 2023-08-10T16:32:32Z

I think I'll make another PR running it on all files and fixing anything that happens once this one is merged

can maybe ignore/exclude the files for now?

…nto add-pre-commit

CoolCat467 · 2023-08-13T22:30:28Z

Mostly just the problems with running pre-commit run --all-files that remains a stopper.

Ok so I decided to fix all the files, that's changed as of d8d6a2d

jakkdl

To avoid another #2711, I think subdirectories under docs/source should be ignored. And not just a matter of updating references to them now, as I think extra empty lines from isort or black is quite unnecessary in those short examples.

This should be set in pyproject.toml (for both isort and black) so they're ignored by manual runs as well. To still get fixes in conf.py and local_customization.py the easiest is probably to explicitly list the three subdirectories.

reformatting notes-to-self shouldn't be a problem afaik

A5rocks

Just a reminder about #2744 (comment) :P

CoolCat467 · 2023-08-15T03:54:48Z

Just a reminder about #2744 (comment) :P

Disabled black and isort for CI autofixes in 316838a (though I do hope we will turn that on)

jakkdl

just a couple small nits

notes-to-self/proxy-benchmarks.py

notes-to-self/graceful-shutdown-idea.py

CoolCat467 · 2023-08-20T02:05:35Z

To ensure this doesn't get too far out of date, I am going to merge this.

A5rocks · 2023-08-20T02:26:52Z

Mind making a followup that updates documentation on contributing to suggest this?

CoolCat467 · 2023-08-20T02:28:39Z

I don't mind at all, I'll be opening a new PR shortly for that.

Add pre-commit

7210ba5

CoolCat467 marked this pull request as ready for review August 7, 2023 02:11

Merge branch 'master' into add-pre-commit

eec0dfa

A5rocks reviewed Aug 8, 2023

View reviewed changes

jakkdl requested changes Aug 8, 2023

View reviewed changes

jakkdl mentioned this pull request Aug 8, 2023

Add type annotations for _ssl.py #2745

Merged

CoolCat467 added 2 commits August 8, 2023 18:53

Remove python version specifier from black

25e0281

Merge branch 'master' into add-pre-commit

b61ba1a

Add codespell

0eaaaa3

jakkdl reviewed Aug 10, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

CoolCat467 added 2 commits August 10, 2023 09:29

Merge branch 'python-trio:master' into add-pre-commit

eee0795

No more skips

caa68e7

jakkdl reviewed Aug 10, 2023

View reviewed changes

CoolCat467 added 3 commits August 13, 2023 17:27

Run pre-commit on all files and fix issues

d8d6a2d

Merge branch 'add-pre-commit' of https://github.com/CoolCat467/trio i…

f7aede8

…nto add-pre-commit

Merge branch 'master' into add-pre-commit

0c05130

CoolCat467 requested a review from jakkdl August 14, 2023 02:49

jakkdl requested changes Aug 14, 2023

View reviewed changes

Revert docs changes and ignore docs formatting in the future

9f00abf

Merge branch 'master' into add-pre-commit

1d8ab83

CoolCat467 requested a review from jakkdl August 14, 2023 16:52

A5rocks reviewed Aug 15, 2023

View reviewed changes

Disable black and isort autoformatting in CI

316838a

jakkdl approved these changes Aug 17, 2023

View reviewed changes

notes-to-self/proxy-benchmarks.py Outdated Show resolved Hide resolved

notes-to-self/proxy-benchmarks.py Show resolved Hide resolved

notes-to-self/graceful-shutdown-idea.py Show resolved Hide resolved

CoolCat467 added 2 commits August 17, 2023 19:07

Fix typo

6aaec43

Merge branch 'master' into add-pre-commit

9978f6e

CoolCat467 merged commit bf847ec into python-trio:master Aug 20, 2023
27 checks passed

CoolCat467 mentioned this pull request Aug 20, 2023

Update contributing.rst to talk about pre-commit #2765

Merged

CoolCat467 deleted the add-pre-commit branch August 20, 2023 02:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pre-commit #2744

Add pre-commit #2744

CoolCat467 commented Aug 7, 2023

codecov bot commented Aug 7, 2023 •

edited

Loading

A5rocks Aug 8, 2023

jakkdl Aug 8, 2023

jakkdl Aug 8, 2023

A5rocks Aug 8, 2023

jakkdl Aug 9, 2023

jakkdl left a comment •

edited

Loading

jakkdl Aug 8, 2023

A5rocks commented Aug 8, 2023

CoolCat467 commented Aug 8, 2023

jakkdl commented Aug 9, 2023

jakkdl commented Aug 9, 2023

jakkdl commented Aug 9, 2023 •

edited

Loading

A5rocks commented Aug 10, 2023

jakkdl commented Aug 10, 2023

jakkdl left a comment

CoolCat467 commented Aug 10, 2023

jakkdl commented Aug 10, 2023

CoolCat467 commented Aug 13, 2023

jakkdl left a comment

A5rocks left a comment

CoolCat467 commented Aug 15, 2023

jakkdl left a comment

CoolCat467 commented Aug 20, 2023

A5rocks commented Aug 20, 2023

CoolCat467 commented Aug 20, 2023

Add pre-commit #2744

Add pre-commit #2744

Conversation

CoolCat467 commented Aug 7, 2023

codecov bot commented Aug 7, 2023 • edited Loading

Codecov Report

A5rocks Aug 8, 2023

Choose a reason for hiding this comment

jakkdl Aug 8, 2023

Choose a reason for hiding this comment

jakkdl Aug 8, 2023

Choose a reason for hiding this comment

A5rocks Aug 8, 2023

Choose a reason for hiding this comment

jakkdl Aug 9, 2023

Choose a reason for hiding this comment

jakkdl left a comment • edited Loading

Choose a reason for hiding this comment

jakkdl Aug 8, 2023

Choose a reason for hiding this comment

A5rocks commented Aug 8, 2023

CoolCat467 commented Aug 8, 2023

jakkdl commented Aug 9, 2023

jakkdl commented Aug 9, 2023

jakkdl commented Aug 9, 2023 • edited Loading

A5rocks commented Aug 10, 2023

jakkdl commented Aug 10, 2023

jakkdl left a comment

Choose a reason for hiding this comment

CoolCat467 commented Aug 10, 2023

jakkdl commented Aug 10, 2023

CoolCat467 commented Aug 13, 2023

jakkdl left a comment

Choose a reason for hiding this comment

A5rocks left a comment

Choose a reason for hiding this comment

CoolCat467 commented Aug 15, 2023

jakkdl left a comment

Choose a reason for hiding this comment

CoolCat467 commented Aug 20, 2023

A5rocks commented Aug 20, 2023

CoolCat467 commented Aug 20, 2023

codecov bot commented Aug 7, 2023 •

edited

Loading

jakkdl left a comment •

edited

Loading

jakkdl commented Aug 9, 2023 •

edited

Loading