-
-
Notifications
You must be signed in to change notification settings - Fork 735
Add isort to pre-commit hooks, package resorting #4647
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
Conversation
Looks like failures due to deprecations in pytest - does this have something to do with #4646? |
Yep, I was trying to figure out what's causing the test suite to start failing (definitely unrelated to the changes here). It looks like there was an issue in a new release of the |
Good to know! In that case, I'll revisit the tests once that is merged. |
#4585 is in, so you should be able to resolve the test failures here by merging |
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.
Thanks @charlesbluca!
I have a similar question to this one over in dask/dask
about making dask
a special third party library so it's imports are just above the distributed
imports instead of being grouped with things like numpy
, tlz
, etc.
cc @jsignell
.pre-commit-config.yaml
Outdated
@@ -1,4 +1,9 @@ | |||
repos: | |||
- repo: https://github.com/pycqa/isort | |||
rev: 5.7.0 |
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.
It looks like release came out a few days ago
rev: 5.7.0 | |
rev: 5.8.0 |
distributed/__init__.py
Outdated
@@ -1,42 +1,42 @@ | |||
from . import config | |||
from . import config # isort: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.
Could you add a small comment here about why we need to skip isort
?
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.
Sure!
Thanks for the review! Distinguishing Dask from other imports makes sense to me, happy to add that in as long as it's consistent with dask/dask#7370. |
We just added a new module which is throwing some |
Sure! Also want to note that a test that failed in the original PR #4600, |
Most of the test failures have something to do with human readable byte formatting not being consistent:
Not sure if this has something to do with the order of imports, as I'm unable to replicate these failures locally. |
This has to do with the work that @crusaderky has been doing in dask/dask#7484 it'll be fixed in #4649 |
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.
Thanks @charlesbluca!
Thanks for the help @jrbourbeau and @jsignell! |
Second pass at #4600, as there were a lot of merge conflicts piling up there.
black distributed
/flake8 distributed