-
Notifications
You must be signed in to change notification settings - Fork 279
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
Handful of isort changes and fixes #2756
Conversation
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.
left two comments to further improve stability of isort results.
Thanks for cleaning up this mess !
So @cphyc showed that there are yet again inconsistent results with isort on travis (see #2759) note that, starting from isort 5.0, the
edit: isort 5.1.3 just came out, looks like it's actively under development for bugfixes these days, so we probably don't want to pin an exact version for now. |
Went ahead and updated as per your suggestion! |
@@ -23,6 +23,7 @@ exclude = doc, | |||
|
|||
# individual files | |||
yt/visualization/_mpl_imports.py, | |||
yt/utilities/command_line.py, |
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.
you should also remove the inline comment in that file (might need to rebase this branch :/)
So, unfortunately, I'm struggling to get this to all work. Can we take this as-is and remove the comment in another PR? |
(As in, specifically, getting the git commit hook to work with the version of isort that also works here etc etc) |
sure, works for me |
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.
LGTM!
note that linting is expected to fail here, and is fixed by the companion PR #2764 |
In the end it turns out this branch isn't necessary after tests passed in #2764, so as we agreed on slack I'm closing this :) |
(turns out there's no "close" button on the mobile app yet !) |
This reformats a few of the files that were still failing, and it
changes the setup.cfg section to be isort rather than tool:isort, which
seems to be the right thing for isort 4.3.