Isort: --check-only return code regression in 4.2.3+ / Add --enforce-whitespace option

[jmagnusson]

In isort 4.2.3 and higher I no longer get a non-0 return code for formatting errors when I pass in --check-only.

Let's say I have a script called mylib.py, with the following contents:

import os
import six

Here's the behavior with different isort versions:

$ pip install isort==4.2.2
$ isort --diff mylib.py
...
@@ -1,2 +1,3 @@
 import os
+
 import six
$ isort --check-only mylib.py
ERROR: /Users/jacob/mylib.py Imports are incorrectly sorted.
$ echo $?
1

$ pip install isort=4.2.3
$ isort --diff mylib.py
...
@@ -1,2 +1,3 @@
 import os
+
 import six
$ isort --check-only mylib.py
$ echo $?
0

$ pip install isort=4.2.4
$ isort --diff mylib.py
...
@@ -1,2 +1,3 @@
 import os
+
 import six
$ isort --check-only mylib.py
$ echo $?
0

$ pip install isort=4.2.5
$ isort --diff mylib.py
...
@@ -1,2 +1,3 @@
 import os
+
 import six
$ isort --check-only mylib.py
$ echo $?
0

My CI tests are no longer failing because of this. Would be awesome if this could be fixed!

[timothycrosley]

Hi @jmagnusson thanks for reporting! I'll see if I can get a hot fix release out soon that resolves this

Thanks!

~Timothy

[bootandy]

Looks like check-only was changed to ignore whitespace (including new lines) here:
https://github.com/timothycrosley/isort/commit/5f6d4bd45e0fa8e3b6b2bea81d3eb7368ee312c3

[timothycrosley]

@bootandy you are correct, so this is indeed an intended change and not a regression. I'll go ahead and mark as closed.

Thanks!

~Timothy

[jmagnusson]

Okay... So how does one check for correctly formatted imports now exactly? It was a very useful feature IMO.

[timothycrosley]

@jmagnusson it still should work for checking most formatting issues, just not things like an lines between imports. This is a tough thing to get right, as peoples definition of what signifies a failure is different from group to group. What would think about a --check-only --enforce-white-space style option?

Thanks!

~Timothy

[jmagnusson]

That would work for me!

[timgraham]

As I asked in #378 which is the PR for the original behavior change, what's the reasoning for it? I'd rather an --ignore-whitespace option to opt-in to the new behavior unless the new behavior is really more sensible.

[jdufresne]

I was recently bit by this as well.

Sometimes a contributor will unknowingly slip in whitespace changes. These used to be caught on the CI server, but that is no longer the case. The end result is that the next time someone runs isort --apply, they will see a lot of unrelated whitespace changes; often in files the developer didn't touch. These style changes could have been caught earlier by the automated tools.

[jmagnusson]

@timothycrosley Would be awesome to have a release with the commit for --enforce-white-space? I'm missing this feature 😢

[Kolyunya]

@timothycrosley can we have a release with the --enforce-white-space option and enforce_white_space configuration parameter please? It's a really must-have feature.