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

Support isort5 #92

Merged
merged 7 commits into from
Aug 7, 2020
Merged

Support isort5 #92

merged 7 commits into from
Aug 7, 2020

Conversation

bnavigator
Copy link
Contributor

@bnavigator bnavigator commented Jul 13, 2020

Hi,

This PR adds support for isort >=5 while retaining the code for isort 4.

The new code parses the unified diff output produced by isort.api.check_file and isort.api.check_code_string.

isort5 only reads one config file and uses a specific order. So the tests need their own temporary dir each. So I started to rewrite the tests and because the self.assertEqual() calls in the unit tests made it hard to debug, I moved everything to pytest fixtures.

Pytest is used by the CI anyway.

I started to work on a PR to support isort 5 before I saw #88 (comment), but I think it is the same approach. Please review.

Fixes #88

Isort > 5 does not have the pyproject extra either.
Just require toml directly for the tests
This let's pytest discover the file on its own.
@rsalmaso
Copy link

Nice, but why keep compatibility with isort < 5? It's a lot of code!
If you need isort < 5 you can live with flake8-isort 3.X

@bnavigator
Copy link
Contributor Author

Nice, but why keep compatibility with isort < 5? It's a lot of code!
If you need isort < 5 you can live with flake8-isort 3.X

From a rolling distrubution package maintainer point of view: Hard sudden changes in requirements create unnecessary friction. The sudden switch from isort 4 to isort 5 API is what triggered this PR and others like pylint-dev/pylint#3725. Downstream packages apparently were not aware of the change and everything that did not declare 'isort<5' broke. Look at all the backreferences in #88.

This PR is about additional support. @gforcada can decide later on to drop isort 4. But I would strongly advise to introduce a DeprecationWarning in Flake8Isort4 and not drop it earlier than on the release after the next. This gives downstream packages the opportunity to update their requirements in time.

@bnavigator
Copy link
Contributor Author

Additionally, if you drop isort 4, you also have to drop Python < 3.6

@bnavigator
Copy link
Contributor Author

bnavigator commented Jul 18, 2020

isort 5.1.1 changed the code how to write the unified diff a bit. Now std.out is not always captured and unit tests fail. Investigating...

@bnavigator
Copy link
Contributor Author

bnavigator commented Jul 18, 2020

The default value of sys.stdout within the output keyword of isort.format.show_unified_diff() does not work with the output capture of flake8-isort. PyCQA/isort@cde7ec8

So the new commit unwraps check_file and check_code_string by calling sort_stream directly with StringIO objects.

  • This PR works with all isort versions
  • If you want to support only isort >=5.1.1, then you can simplify further by bnavigator@68032b2
  • If you really want to drop isort 4 completely, merge bnavigator@fb9ab3c

setup.py Show resolved Hide resolved
@gforcada
Copy link
Owner

gforcada commented Aug 7, 2020

😱 I never actually said anything! 🤦 sorry for that @bnavigator and all that have made comments and suggestions! 🙇 Great job!!! 💯

I did a first review some weeks ago, but I will do a second pass and make a release, even an alpha or beta one so whoever wants a release with isort 5 support at least does not have to roll their own releases or use git checkouts 👍

yield line_num, self.isort_add_unexp


Flake8Isort = Flake8Isort5 if hasattr(isort, 'api') else Flake8Isort4
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 💯

@gforcada gforcada merged commit fee5f69 into gforcada:master Aug 7, 2020
@gforcada
Copy link
Owner

gforcada commented Aug 7, 2020

@bnavigator thanks a lot! 🙇 💯 flake8-isort 4.0.0a0 is out!! 🎉 🥳

@gforcada
Copy link
Owner

gforcada commented Aug 7, 2020

Please give feedback with this release and I will make a final a few days/weeks later if no negative (and some positive) feedback is received 👍

@bnavigator
Copy link
Contributor Author

bnavigator commented Aug 7, 2020

Please give feedback with this release

Looks good! 👍

@thomasgilgenast
Copy link

Please give feedback with this release

I've been using this PR for a couple weeks and it's been working perfectly in my setup with isort==5.2.2 and flake8==3.8.3. Thanks for the PR and new release!

@rsalmaso
Copy link

rsalmaso commented Aug 7, 2020

using with flake8==3.8.3 and isort==5.3.2: 👍

@gforcada
Copy link
Owner

Thanks for all the feedback, you can do a last update to bump flake8-isort to 4.0.0! 🎉

Again, thanks to all that contributed on bringing isort 5 compatibility! 🙇 ✨

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.

Not working with isort new release v5.0.0
5 participants