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

Pin static-checking versions and add pre-commit Action #813

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

Conversation

hristog
Copy link
Contributor

@hristog hristog commented Mar 28, 2021

Updates, associated with this PR:

  • The original intention was to only pin code checking-related versions to their dask and distributed equivalents (as per Pin to a particular black tag in pre-commit dask#7256 and Pin black pre-commit distributed#4533).
  • However, isort and black can apparently be stuck in a never-ending loop of proposing updates to the same file back and forth.
  • Given the latter, a quick search has revealed that there have been recent efforts to facilitate the easier interoperability of black and isort, namely a --profile=black option, which is available in recent isort versions (but not in 4.3.21, which is why this PR proposes bumping the isort version up to 5.8.0, released on March 20th 2021). I appreciate that some previous efforts to enable isort and black to play nicely together are present in the isort 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).

@hristog hristog marked this pull request as ready for review March 28, 2021 14:01
@hristog hristog changed the title Pin code check versions (black==20.8b1 and flake8==3.8.3) Pin code-checking versions (black==20.8b1 and flake8==3.8.3) Mar 28, 2021
@hristog hristog changed the title Pin code-checking versions (black==20.8b1 and flake8==3.8.3) Pin versions (black==20.8b1, flake8==3.8.3 and isort==5.8.0) Mar 28, 2021
@hristog hristog changed the title Pin versions (black==20.8b1, flake8==3.8.3 and isort==5.8.0) Pin black==20.8b1, flake8==3.8.3 and isort==5.8.0 Mar 28, 2021
@hristog
Copy link
Contributor Author

hristog commented Mar 28, 2021

@jsignell, I've just discovered your dask/dask#7370 (I can see that the isort version there is 5.7.0, and also that the PR is in a draft state).
Could you, please, have a look at this PR when you get a chance?
I'll be happy to update this PR to isort==5.7.0 (instead of the proposed 5.8.0), if that's your preference.

@jsignell
Copy link
Member

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

Copy link
Member

@jsignell jsignell left a 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.

setup.cfg Show resolved Hide resolved
@hristog
Copy link
Contributor Author

hristog commented Mar 29, 2021

My main suggestion is that in dask/dask, there is a github action that runs pre-commit.

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=
Copy link
Contributor Author

@hristog hristog Mar 29, 2021

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.

@jsignell
Copy link
Member

GitHub Actions... could be done as part of this PR, or if it requires maintainer access to the repository?

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

@hristog
Copy link
Contributor Author

hristog commented Mar 30, 2021

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!
I'll update this PR a bit later today, to incorporate your suggestion.

@hristog hristog changed the title Pin black==20.8b1, flake8==3.8.3 and isort==5.8.0 Pin static-checking versions and add pre-commit Action Mar 30, 2021
- flake8
- isort==4.3.21
- flake8==3.8.3
- isort==5.8.0
- msgpack-python ==0.6.2
- multipledispatch
- mypy
Copy link
Contributor Author

@hristog hristog Mar 30, 2021

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.

@hristog
Copy link
Contributor Author

hristog commented Apr 9, 2021

@jameslamb has brought up a very good point in #822 (comment) about considering pinning the static checker versions in

test_requires = [
as well (if my understanding has been correct).
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 mypy to setup.py?

@hristog
Copy link
Contributor Author

hristog commented Apr 9, 2021

pinning the static checker versions

One potential disadvantage of that (i.e., pinning minimum versions for isort, flake8 and friends) might be that the local dev environment would have to be modified which, in turn, might cause inconvenience when the same underlying tools would need to be utilized in the context of other projects.

On the other hand, that's already what happens with respect to the non-testing package requirements:

install_requires = [

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.

None yet

3 participants