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

ISort fails to recognise whitespace-only line as a blank line #811

Closed
bignose-debian opened this issue Jan 26, 2019 · 6 comments
Closed
Labels
bug Something isn't working

Comments

@bignose-debian
Copy link

bignose-debian commented Jan 26, 2019

When processing lines, ISort fails to recognise some whitespace-only lines as blank lines.

isort-blank-lines.py.txt
The attached file contains this content:

    import os
    import sys

    
    print("Hello, world!")

When ISort process that input file, the two blank lines are not recognised correctly, and ISort mandates these changes:

    $ python3 -m isort --check-only --diff ./isort-blank-lines.py 
    ERROR: /home/bignose/Projects/python/isort-blank-lines.py Imports are incorrectly sorted.
    --- /home/bignose/Projects/python/isort-blank-lines.py:before   2019-01-26 21:14:47.421581
    +++ /home/bignose/Projects/python/isort-blank-lines.py:after    2019-01-26 21:15:18.837737
    @@ -1,6 +1,4 @@
     import os
     import sys
     
    --
     print("Hello, world!")

The input file is valid, because the second blank line (containing only U+000C FORM FEED) is a valid blank line in Python code.

ISort should use the same meaning of “blank line” as the Python compiler.

@timothycrosley timothycrosley added the bug Something isn't working label Mar 11, 2019
@timothycrosley
Copy link
Member

Thanks for reporting this issue: in the latest release of isort these characters are correctly ignored if --ignore-whitespace is set.

Thanks!

~Timothy

@bignose-debian
Copy link
Author

Thanks for the update.

I think that this bug is not resolved. The --ignore-whitespace option tells ISort to ignore changes in whitespace. That's unrelated to this bug report.

With or without that option, ISort should be counting blank lines the same way Python does.

@timothycrosley
Copy link
Member

isort is not built to be a checker, it is built to be a reformatter. It is not saying that blank line is incorrect, it's simply saying it when it reformats it will take it out (since it strips everything Python does by default using .strip in the blank lines below imports - essentially Python is categorizing it in this way as whitespace). Is there a functional reason to have that style of new line? Is isort breaking anything by removing it?

Thanks!

~Timothy

@bignose-debian
Copy link
Author

it strips everything Python does by default using .strip in the blank lines below imports

Thanks for the clarification.

In that case, this is still an unresolved bug. The line should not be stripped, and should not be ignored. It is a valid blank line and should remain, because it is a valid blank line by Python's syntax rules.

@bignose-debian
Copy link
Author

This is still an unresolved bug; can we get attention to this, and get it fixed?

According to Python syntax rules for blank lines, U+000C is a valid character on a blank line:

A logical line that contains only spaces, tabs, formfeeds and possibly a comment, is ignored (i.e., no NEWLINE token is generated).

According to Python syntax rules for indentation, U+000C do not constitute indentation space:

A formfeed character may be present at the start of the line; it will be ignored for the indentation calculations above.

So, a line consisting of U+000C should not be stripped, and should not be altered.

@bignose-debian
Copy link
Author

Here is a test case (added to 'tests/unit/test_isort.py') which currently fails:

modified   tests/unit/test_isort.py
@@ -3251,6 +3251,13 @@ def test_strict_whitespace_no_closing_newline_issue_676(capsys) -> None:
     assert out == ""
 
 
+def test_leading_formfeed_issue_811(capsys) -> None:
+    test_input = "import os\n\f\nfrom django.conf import settings\n"
+    assert api.check_code_string(test_input)
+    out, _ = capsys.readouterr()
+    assert out == ""
+
+
 def test_ignore_whitespace(capsys) -> None:
     test_input = "import os\nfrom django.conf import settings\n"
     assert api.check_code_string(test_input, ignore_whitespace=True)

That test case should pass, when this bug is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants