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 2ba782f

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 f5ef546 (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 (2ba782f), 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 f5ef546 to be consistent because it adds a newline at the end of the output in both cases.
However, commit 2ba782f (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:

isort/isort/output.py

Line 29 in 37d6d52

formatted_output: List[str] = parsed.lines_without_imports.copy()

where formatted_output has the lines_without_imports and then later on:

isort/isort/output.py

Line 550 in 37d6d52

def _normalize_empty_lines(lines: List[str]) -> List[str]:

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:

isort/isort/output.py

Line 550 in 37d6d52

def _normalize_empty_lines(lines: List[str]) -> List[str]:

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:

isort/tests/unit/test_isort.py

Lines 70 to 74 in 1e6db95

	test_input = "import sys\nimport os\nimport myproject.test\nimport django.settings"
	test_output = isort.code(test_input, known_first_party=["myproject"])
	assert test_output == (
	"import os\n" "import sys\n" "\n" "import django.settings\n" "\n" "import myproject.test\n"
	)

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