Newline not always added at the end when input has no newline

[glostis]

Hi,

First of all, thanks for the great tool which I have been using daily for the past 2 years! Thanks in particular for the 5.X release with the --profile black option which is very handy!

I have been observing what I think is an inconsistency in the output of isort when run with a string from stdin. A git bisect shows that this inconsistency was introduced with commit 2ba782fb1a74294f2a67cd64fe4b6bf98dddba30

Consider the following snippet of code:

import isort

str1 = "import os\nprint(os.environ)"
str2 = "import os\ndef main():\n    print(os.environ)"
for string in [str1, str2]:
    print("Input:")
    print(repr(string))
    print("Output:")
    print(repr(isort.SortImports(file_contents=string).output))
    print("----")

When run at commit f5ef546ede9450b19d7c615ab7e7c833f88a7dcf (i.e. just before the offending commit), it prints:

Input:
'import os\nprint(os.environ)'
Output:
'import os\n\nprint(os.environ)\n'  <- newline added
----
Input:
'import os\ndef main():\n    print(os.environ)'
Output:
'import os\n\n\ndef main():\n    print(os.environ)\n' <- newline added
----

When run at the offending commit (2ba782fb1a74294f2a67cd64fe4b6bf98dddba30), it prints:

Input:
'import os\nprint(os.environ)'
Output:
'import os\n\nprint(os.environ)\n' <- newline added
----
Input:
'import os\ndef main():\n    print(os.environ)'
Output:
'import os\n\n\ndef main():\n    print(os.environ)' <- no newline added
----

I consider commit f5ef546ede9450b19d7c615ab7e7c833f88a7dcf to be consistent because it adds a newline at the end of the output in both cases. However, commit 2ba782fb1a74294f2a67cd64fe4b6bf98dddba30 (up til the HEAD of develop as of writing) is inconsistent because it adds a newline in the str1 case, but not in the str2 case.

[timothycrosley]

Thanks for alerting to this inconsistency!

Some background: This is mostly intentional. isort adding a new line at the end of the file was an unintentional side effect of isort processing full files < 5.0.0. Now isort only processes the import sections (defined as contiguous sections of imports / comments and only the first line of non import code to determine how many lines after imports to display). The reason the new line is added in the simple example is simply because it ends with a contiguous import section according to this logic. I think the right solution here is to ensure that isort doesn't change the line ending behaviour even in these simple cases.

[anirudnits]

@timothycrosley I believe the problem is here: https://github.com/PyCQA/isort/blob/37d6d52b343229179f6f6d96d2247dead180d7bd/isort/output.py#L29 where formatted_output has the lines_without_imports and then later on:

https://github.com/PyCQA/isort/blob/37d6d52b343229179f6f6d96d2247dead180d7bd/isort/output.py#L550 where an extra newline is added regardless.

My initial strategy was to only process the import_lines and later just append the lines_without_imports and returning it. But that doesn't work, especially when two extra lines need to be added for function definitions and stuff. Any suggestions?

Update: I have come up with a somewhat shaky way of managing it(didn't really test it yet) by completing bypassing the: https://github.com/PyCQA/isort/blob/37d6d52b343229179f6f6d96d2247dead180d7bd/isort/output.py#L550 and rather just appending the lines_with_imports and lines_without_imports part after all the normal processing that is.

[anirudnits]

is there anybody who is working on this presently? otherwise going to assign myself to this.

[anirudnits]

Sorry I haven't worked or updated this issue in some time. My strategy is to add any extra flag end_of_file to signify the end of stream and if so then there's no need to add an extra newline at the end of that import section. This works for the 2 cases specified in this thread, but fails a lot of existing test cases such as:

https://github.com/PyCQA/isort/blob/1e6db95820f4341712fb7fd9e993597543480a0b/tests/unit/test_isort.py#L70-L74

where a succeeding newline is assumed at the end of the import section. I believe this test case(along with others) is part of the inconsistency and need to be changed. I just wanted to share this before I actually go and change existing test cases to resolve this issue.

@timothycrosley thoughts?

[timothycrosley]

@anirudnits I think you're spot on with the approach and am sorry I haven't looped back on this issue in some time! Are you still able to work on it?

Either way thanks for your research and analysis