Support both isort 4 and isort 5

[dbaty]

Description

isort 5 has been released a few days ago (2020-07-04) but its API is not compatible anymore with pylint. We could switch to isort 5 (and require isort>=5,<6) as I did in #3724, but isort 5 dropped the compatibility with Python 3.5, which pylint still supports. So a better option could be this pull request that supports both isort 4 and isort 5 via a small wrapper.

All tests run with isort 5, except the Python 3.5 build that installs isort 4 (see bc0a82a#diff-b91f3d5bd63fcd17221b267e851608e8R61-R62). One of the test is manually skipped there, as it seems complex to have a different expected output depending on the installed version of isort (see bc0a82a#diff-37021361f4a7fe2764d8ff4537f22facR81-R84).

I switched the style of pylint's own import statements and its tests to isort 5 in a separate commit.

(Since pylint currently pins the version of isort to ">=4.2.5,<5", I don't think there is anything urgent, despite the comments in #3722...)

Type of Changes

	Type
✓	🔨 Adapt to changes in dependencies

Related Issue

Closes #3722

Steps

• Add yourself to CONTRIBUTORS if you are a new contributor (already done)
• Add a ChangeLog entry describing what your PR does.

[coveralls]

Coverage increased (+0.07%) to 90.696% when pulling b29385c on Polyconseil:dbaty/isort_both into 707fc46 on PyCQA:master.

[alexchandel]

Nice cleanup.

[timothycrosley]

I think, you'll want to use extra_standard_library here if you the behaviour to stay the same as the isort4 version: https://timothycrosley.github.io/isort/docs/upgrade_guides/5.0.0/#known_standard_library

[jonathanunderwood]

It would be great if we could get this into the next release - a number of projects I am working on are blocked from isort5 adoption by pylint. Hopefully @timothycrosley 's point about extra_standard_library can be tackled ahead of merge, though.

[dbaty]

Sorry, I did not have time to look at Timothy's comment, and will be away from a keyboard for a week. Anyone is welcome to amend the pull request in the meantime. :)

As for the case in point:

I think, you'll want to use extra_standard_library here if you the behaviour to stay the same as the isort4 version:

The question is: do we want to keep the behaviour of isort 4? I confess I am not too familiar with the changes, but I'd say that we should keep in line with the default behavior of latest isort version (i.e. isort 5). This may generate new pylint warnings, but that's the cost of using the latest version of a dependency. And I think it's reasonable.

[timothycrosley]

I took a closer look and I think @dbaty is right that it should be okay. I may suggest however that an extra-standard-library option be added, as right now if someone used to do:

pylint myproject --known-standard-library sre_parse

Now, nothing but sre_parse would match as standard library. os for instance would be seen as third party. This is conceptually okay, but like the isort project itself decided, it might be nice to at least allow users to have a way to add individual modules to stdlib in the case a new version of Python comes out inbetween isort or pylint releases (which is a more common use case).

pylint myproject --extra-standard-library sre_parse

It also in my mind depends on whether or not the pylint project sees known-standard-library as part of its user facing API that happens to be using isort underneath (in which case likely wants to keep the users behaviour the same regardless of isort releases) or if it sees it as simply a pass-through to isort.

[jacobeatsspam]

please

[sunlin7]

Bothered by the incompatible message, look forward for this MR be merged.

[akaihola]

FWIW I've been using this branch of Pylint together with isort 5.x for a month now, without problems.

[chrisjbillington]

Arch Linux has just updated to isort 5.2.2, so Pylint is broken on Arch until there is a release with this fix.

https://bugs.archlinux.org/task/67555

Edit: Arch is now shipping this patch

[dbaty]

Hi @timothycrosley, thanks for your insight and precious advice.

It also in my mind depends on whether or not the pylint project sees known-standard-library as part of its user facing API that happens to be using isort underneath (in which case likely wants to keep the users behaviour the same regardless of isort releases) or if it sees it as simply a pass-through to isort.

That's a very good point. And the target implementation in pylint (this very PR) depends on the answer to this.

I will assume the following: most users who specify known-standard-library with isort 4 want what extra-standard-library means in isort 5. @timothycrosley : would you say this assumption is correct? You certainly have a much deeper and wider understanding of isort usage. (For those following along, see Timothy's comment here and isort 5 migration guide.)

Considering that, I see two ways to deal with this change in pylint:

1. Provide both options in pylint (as pass-throughs): known-standard-library and extra-standard-library. This is the most flexible because it allows more control on isort settings. However, according to the assumption above, most users who used known-standard-library will have to change their pylint configuration to use extra-standard-library instead. In other words, this would be a breaking change in pylint for most users of known-standard-library.

2. Keep known-standard-library as the only option in pylint. It would map with known-standard-library if isort 4 is installed ; or with extra-standard-library if isort 5 is installed. Most users who currently use known-standard-library will be happy, as this will not break their expectation. However, for (the supposedly rare) users who want greater control and use the new behaviour of known-standard-library from isort 5, pylint options will not allow that.

I am inclined to favor option 2 because:

• it's not a breaking change;
• the (supposedly few) users who will be left behind, have custom needs. pylint cannot implement all options from isort. If you need something special, you need to disable the wrong-import-order check in pylint and run isort manually with a proper isort configuration file. (I might be biaised because that's what we already do in my company's projects.)

This is an important decision that warrants the point of view of a real maintainer (and not only contributors like me).

[dbaty]

Thanks @Pierre-Sassoulas for the review. :) I addressed your comments and actually implemented what you and I see as the best option. I force-pushed to the branch to "ease" any other review (and so that the branch can be merged as is, hopefully). The message of the main commit (5fff33f 03053c0) and the changelog entry have been expanded. I hope they are clear enough and not too verbose.

Note for other reviewers: as another commenter noted above, this pull request has 3 separate commits. I suggest to review them one by one (through the "Commits" tab, not the "Files changed" tab).

(Updated 2020-08-18 06:48 UTC: update to the link of the main commit since I push-force again to update the "What's New" page, as recommended by PIerre-Sassoulas. I also fixed the typo in the commit message, report by bnavigator.)

[bnavigator]

5fff33f :

- isort 5. Users that really the _new_ meaning of
+ isort 5. Users that really want the _new_ meaning of

[Pierre-Sassoulas]

Thanks, the commit message is very clear. By the way I forgot, that you should also add a summary in https://github.com/PyCQA/pylint/blob/master/doc/whatsnew/2.6.rst, as this is a notable change and the third paragraph with known-standard-library =extra-standard-library seems important to have somewhere more visible than the commit itself.

I have no estimate of the time the next version will be released as I can't release myself. We probably want to release ASAP because of the number of persons that want this and also because of #3761 . (@AWhetter or @hippo91 you seem to have maintainer right on pypi, could you do something about it?)

[dbaty]

By the way I forgot, that you should also add a summary in https://github.com/PyCQA/pylint/blob/master/doc/whatsnew/2.6.rst,

It's done (in 03053c0) and I updated the branch. I also fixed the typo (in this commit message) reported by @bnavigator.

[basnijholt]

Thanks for the great work!

Any idea when this will make it into a (patch) release?

[hippo91]

@dbaty and other contributors, thanks a lot for this good job!
I indeed have maintainer right on pypi but never made a version yet.
I would strongly prefer to wait for news about @PCManticore before running into it. @AWhetter what do you think about it?

[AWhetter]

In the absence of @PCManticore, yes let's do a new release and get this out soon. @hippo91 how do you feel about doing your first release? The release process is documented here: https://github.com/PyCQA/pylint/blob/master/doc/release.txt

[hippo91]

@AWhetter ok lets do this. I will try the process tomorrow.

[Pierre-Sassoulas]

Thank you both ! I did not know about this doc. If I understand correctly I could have released myself just by creating a tag. Sorry for pinging you unnecessarily, it won't happen next time :)

[hippo91]

@AWhetter @Pierre-Sassoulas
I'm almost done with the creation of the version 1.6.0. However i encountered a small problem with Travis and Pypi. Here is the end of the log on Travis:

Uploading pylint-2.6.0-py3-none-any.whl

100%|██████████| 329k/329k [00:00<00:00, 569kB/s]  

NOTE: Try --verbose to see response content.

HTTPError: 400 Bad Request from https://upload.pypi.org/legacy/

I don't know why this error occurs but i will investigate tomorrow, if you have some idea...
Do you know how i can relaunch the Travis procedure?

[AWhetter]

hmm I don't know why that is. But the documentation that I linked to also includes steps for performing the release manually. You should be able to perform those steps despite whatever Travis tried to do.

[Pierre-Sassoulas]

@hippo91, I'm afraid you have to be logged as yourself in order to relaunch your repository's job (for pylint it would be as PyCQA ?). The button for restart should be at the top right side check one of your own repository. Manual release seems to be the only way now (but should not be a problem with maintainer right). Do you have the right to give me the pypi right ? If such a case happen again I would not have to ping everyone during summer holiday :D

[AWhetter]

I don't have those management permissions unfortunately.

[hippo91]

I finally succeed in making 2.6.0.
The error in upload what due to a syntax error in the README.rst. Pypi doesn't accept upload if it cannot render the description correctly.
For information, it was possible to relaunch the build in Travis but after switching the github dashboard context from my account to PyCQA one.
I hope everything will be ok with this version.

[jacobeatsspam]

Thank you!