-
-
Notifications
You must be signed in to change notification settings - Fork 257
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
Pin static-checking versions and add pre-commit Action #813
base: main
Are you sure you want to change the base?
Conversation
@jsignell, I've just discovered your dask/dask#7370 (I can see that the |
I imagine I just picked the latest version of isort when I opened that PR on dask, so there is no special magic to that version :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good to me. My main suggestion is that in dask/dask, there is a github action that runs pre-commit. That is a pretty nice way to capture the linting config in one place.
Thanks for checking this out, @jsignell! Apologies - I'm not very familiar with GitHub Actions, and wanted to double-check if implementing your suggestion could be done as part of this PR, or if it requires maintainer access to the repository? |
@@ -20,8 +20,6 @@ force_grid_wrap=0 | |||
combine_as_imports=True | |||
line_length=88 | |||
profile=black | |||
skip= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the new profile=black
option, skipping docs/source/conf.py
is no longer necessary.
It can be done as part of this PR or as a follow-on if you prefer. Shouldn't require any special access. This is how the dask pre-commit action is set up https://github.com/dask/dask/blob/main/.github/workflows/pre-commit.yml |
Understood - thank you very much, @jsignell! |
ci/environment-3.6.yaml
Outdated
- flake8 | ||
- isort==4.3.21 | ||
- flake8==3.8.3 | ||
- isort==5.8.0 | ||
- msgpack-python ==0.6.2 | ||
- multipledispatch | ||
- mypy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reproducible/predictable CI runs, the mypy
versions in these files should probably be pinned as well. Please, let me know if you've got any objections.
@jameslamb has brought up a very good point in #822 (comment) about considering pinning the static checker versions in Line 28 in f5e5bb4
Also, I've just noticed that mypy is missing from setup.py.
@jrbourbeau, when you get a chance, could you confirm if you've got any objections, with respect to pinning optional-dependency versions and adding |
One potential disadvantage of that (i.e., pinning minimum versions for On the other hand, that's already what happens with respect to the non-testing package requirements: Line 13 in f5e5bb4
|
Updates, associated with this PR:
isort
andblack
can apparently be stuck in a never-ending loop of proposing updates to the same file back and forth.black
andisort
, namely a--profile=black
option, which is available in recentisort
versions (but not in4.3.21
, which is why this PR proposes bumping theisort
version up to5.8.0
, released on March 20th 2021). I appreciate that some previous efforts to enableisort
andblack
to play nicely together are present in theisort
section of setup.cfg, but it somehow feels more logical to aim to employ recent versions in addition to that, unless there exist some associated constraints (CI environment-related or otherwise).