Add option to allow overriding isort's skip_gitignore option

[gschaffner]

Hi!

I was profiling flake8 and found that ~90% of the runtime was just isort waiting for git subprocesses in order to implement skip_gitignore = True. (isort doesn't use libgit2 or anything, it just starts short-lived subprocesses targeting the git binary.)

skip_gitignore = True is not very useful when running flake8-isort since flake8 uses its own ignore list already. This PR adds an option --isort-no-skip-gitignore that allows users to force skip_gitignore = False when running flake8-isort without having to remove skip_gitignore = True from their isort config.

On the library I was profiling, --isort-no-skip-gitignore brought the flake8 runtime from ~50 s to ~5 s. Note that this measurement is on a Linux box. On Windows the speedup provided by this option appears to be much larger (apparently subprocess is extremely slow on Windows?). I actually did not finish the measurement on a Windows box because without --isort-no-skip-gitignore, flake8 was taking > 5 min.

Note that skip_gitignore was added in isort 5.2, so I added the option to Flake8Isort5 rather than Flake8IsortBase.

[gforcada]

@gschaffner thanks for this PR!! ✨

First, sorry for the delay on the response! 😱 I've been rather busy these last days 🤷🏾

Wow! this measurements are really astonishing! From 50s to 5s! 🐎

I'm a bit hesitant on adding the option though 🤔 There are quite a few issues/PR that ask for other settings, and I'm not quite happy of adding all that complexity, this plugin is mostly gluing isort and flake8 together, so if you want to control how it outputs, what it does, etc you should do that on flake8 itself, and if you want to control the way imports are sorted that should be isort settings, not flake8-isort itself...

So, I would rather say that rather than adding the option, we always use this option right away... is there any reason not to do so? 🤔

Another interesting point that you bring is isort 4 and isort 5. I see that there are pre-releases for a version 6 of isort, and last release of isort 4 is June 2019.

Maybe it is time to make a breaking release of flake8-isort and remove isort 4 support, specially if in a few weeks we will get requests to support isort 😓

I'm not asking you to do that 😄 your PR is mostly fine 👍🏾 as said, I would remove the option if there is really no good reason to keep it.

The other changes I will take care myself (although I'm happy to review other PRs adding the changes mentioned above 😄 ).

Again, sorry for the delay on this review! 🙇🏾

[gschaffner]

hi!

Wow! this measurements are really astonishing! From 50s to 5s! 🐎

the speedup from this option changes a bit when using isort 5.11, which was released yesterday:

• linux:

  • isort 5.11: reduced from ~5.3 s to ~3.7 s
  • isort 5.10: reduced from ~50 s to ~5 s

• windows:

  • isort 5.11: reduced from ~32 s to ~10 s
  • isort 5.10: reduced from > 10 min to ~10 s

this is likely largely due to PyCQA/isort#1900.

so while the speedup is not as drastic when used with isort >= 5.11, skip_gitignore = False can still constitute a meaningful speedup, and a large speedup on windows.

I'm a bit hesitant on adding the option though 🤔 There are quite a few issues/PR that ask for other settings, and I'm not quite happy of adding all that complexity, this plugin is mostly gluing isort and flake8 together, so if you want to control how it outputs, what it does, etc you should do that on flake8 itself, and if you want to control the way imports are sorted that should be isort settings, not flake8-isort itself...

I absolutely agree with you in general here. however, this particular option causes flake8-isort to behave differently than isort, so users can't always put skip_gitignore = false their isort config instead. users of skip_gitignore = true cannot always just set it to false (to speed up flake8-isort) since doing so will also affect what isort reformats when it is run outside of flake8-isort.

So, I would rather say that rather than adding the option, we always use this option right away... is there any reason not to do so? 🤔

having this option as always enabled (or as enabled by default) would cause flake8-isort to, by default, behave differently than isort --check.

I think that anything that causes flake8-isort to behave differently than isort should probably not be enabled by default. most users probably assume that flake8-isort will respect isort's configuration. this is a pretty reasonable thing to assume, but enabling --isort-no-skip-gitignore by default would violate this assumption. (it would also be a breaking change.)

making the option opt-in rather than always-on or opt-out means that it will not be enabled unless developers first read the documentation and take the time to think "yes, I understand that enabling this option means that flake8-isort will report files that isort --check would not report. I want the speedup from this though, so I will be careful that in places where the return code is checked automatically (e.g. in CI), flake8 is only called on files that are not gitignored."

forcefully enabling --isort-no-skip-gitignore would cause people's CI to fail. the only way they could fix it is by duplicating information from their gitignores into their flake8 I ignore config, e.g. via

[flake8]
per-file-ignores =
    ...: I

or by duplicating information from their gitignores into their isort config, e.g. via

[tool.isort]
extend_skip_glob = [
    "...",
]

I think forcing users to duplicate config like this is bad for a couple reasons:

• config has to be duplicated!

• most .gitignores in the repository will need to include a comment that says "when updating this, you also need to figure out if you need to update flake8/isort's config too!"

and even if users DO duplicate config like this, flake8-isort still couldn't (in all cases) be made to work the way it would if it respected skip_gitignore = true:

• gitignores outside of the repository (e.g. .git/info/exclude, ~/.config/git/ignore, and gitignores that are themselves gitignored) will need to be manually reviewed and added to the isort/flake8 config. however, the isort/flake8 config is under version control, and these gitignores aren't (since they are intended to differ from developer-to-developer)!

• I believe that the glob syntax understood by flake8 and isort is not as powerful as the syntax understood in gitignores. for example, I don't think per-file-ignores or extend_skip_glob support pattern negation. if this is the case, copying some gitignore configs into e.g. per-file-ignores or extend_skip_glob is probably impossible without resorting to manually including/excluding specific files instead of using a pattern-matching syntax. of course, this creates a new problem: this copied-from-gitignore config now sometimes needs to be updated when new files are added to the repository, even when no .gitignore has actually changed!

sorry for the long message :P — let me know if it's confusing or phrased poorly!

[gschaffner]

Maybe it is time to make a breaking release of flake8-isort and remove isort 4 support

I'm all for this if it makes flake8-isort simpler to maintain :). those people still using isort 4 can just use an older flake8-isort with it. if the packaging config is updated to require isort > 5, nothing will break for isort 4 users.

[gforcada]

Thanks for the long answer actually 😄 I can understand much better now the idea behind the change 👍🏾

Nice timing with the isort release 🚀 then I'm even more hesitant to add this option 🤔 Let me explain myself:

The idea of adding this option in flake8-isort is to change isort own configuration, but doing it so only on a few scenarios, when one is aware of introducing this change for a single run (CI?).

For that, wouldn't you achieve the same by directly modifying temporary isort's own configuration? ✨

So rather than doing the change through flake8-isort you could already change isort configuration and restore that configuration afterwards.

If the intended usage is CI, one could have the option in the configuration, but commented out, and CI, before running flake8 could un-comment that specific line.

As the quote says, the best line of code is the one you don't have to write 😅

My train of thought is: if there is a not so complicated way to get the same effect without having to write more code on flake8-isort, all the better 😄

[gschaffner]

thanks for the reply!

The idea of adding this option in flake8-isort is to change isort own configuration, but doing it so only on a few scenarios, when one is aware of introducing this change for a single run (CI?).

rather than using --isort-no-skip-gitignore only for specific flake8 runs (e.g. CI), at work we are using --isort-no-skip-gitignore in our permanent flake8 config. this is so that flake8 calls made by developers get the big speedup. speeding up flake8 calls made by developers who are actively waiting for flake8 to finish is, for us, much more important than speeding up flake8 calls made by CI in the background.

For that, wouldn't you achieve the same by directly modifying temporary isort's own configuration? sparkles

this would only confer the speed benefit when calling flake8 via the CI system (or via another script that temporarily modifies isort's configuration when calling flake8). the speed benefit would not be conferred to developers running flake8 (unless they were forced to run flake8 via a wrapper script that handles the temporary isort changes).

another problem with temporarily changing isort's configuration is that CI systems often behave differently when config files have been changed. for example, I work with some CI systems that will check all files if a relevant config has been changed, but if no relevant config has been changed they will take a shortcut to save time and check only changed files (since other files are guaranteed to still pass checks). temporarily changing isort's configuration would break this shortcut and would cause CI times to increase drastically (by more than an order of magnitude in some projects I work on).

As the quote says, the best line of code is the one you don't have to write 😅

My train of thought is: if there is a not so complicated way to get the same effect without having to write more code on flake8-isort, all the better 😄

absolutely!! but I think adding one line (isort-no-skip-gitignore = true) to their config is a lot smaller/simpler/less work to set up and maintain than having to

• write and maintain a .isort.cfg/pyproject.toml/setup.cfg/tox.ini transformer script that sets skip_gitignore = False. (a minor point: this script also needs to do some directory walking since a project may be using multiple isort config files (https://pycqa.github.io/isort/docs/configuration/config_files.html#supporting-multiple-config-files-in-single-isort-run))
• (possibly) write and maintain workarounds for the CI slowdown problem discussed above. (depending on the CI system, it may not be possible to work around this.)

in my opinion adding ~30 lines of straightforward code to flake8-isort is much simpler than that whole mess, but at the end of the day it is of course your call :)

[gforcada]

Oh my, my comment went 🗑️ 🤦🏾

Anyway, thanks again for taking the time to explain things ✨

Seems that you are dealing with quite a lot of complexity already, so I would still err on the side of, "that's a too edge case for flake8-isort", but at the same time, it is not that much code as you say 👍🏾

Let's get that merged then 😄

Would you mind adding a line on CHANGES.rst as well as a short explanation on README.rst about this new option? 🙏🏾

[gschaffner]

sorry for the delay!

Would you mind adding a line on CHANGES.rst as well as a short explanation on README.rst about this new option? 🙏🏾

done. I have also rebased onto master to resolve conflicts. I bumped the minor version (I assumed that flake8-isort follows Semantic Versioning), but if flake8-isort doesn't follow SemVer than I can undo that.

[gforcada]

Thanks for the version bump, though, as I release with zest.releaser it already asks for it when doing the release ✨

[gforcada]

I was about to release, but given that we dropped isort 4.x support, and that's a breaking change, we might as well add flake8 6.0.0 support so we don't have to make two breaking releases in a row 😅

IMHO making thousands of breaking releases (i.e. setuptools) makes semver not so important anymore, for the consumers at least. Of course setuptools and flake8-isort are two different projects with completely different target audiences and expectations 😄

[gschaffner]

thanks again!

BTW, I believe that dropping isort 4.x support will not be a breaking change (since you restricted the isort version in 4d0cdb4). isort 4 users will not be affected by the new release.