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

Fix --diff output when encountering EOF #1328

Merged
merged 1 commit into from Apr 5, 2020

Conversation

akien-mga
Copy link
Contributor

@akien-mga akien-mga commented Apr 2, 2020

split("\n") includes a final empty element "" if the final line
ends with \n (as it should for POSIX-compliant text files), which
then became an extra "\n".

splitlines() solves that, but there's a caveat, as it will split
on other types of line breaks too (like \r), which may not be
desired.

Fixes #526.

Thanks to @benkuhn for the well-debugged issue.

@akien-mga
Copy link
Contributor Author

As mentioned, splitlines() works well but it might be doing more than wanted here. I'm not a Python guru, but I'm sure someone may have an elegant idea for ensuring that we can convert a string with \n to a list of lines with trailing \n, without having this extra empty line.

@akien-mga
Copy link
Contributor Author

Fixed the test which was also exhibiting this bug (and thus failing after the fix).

`split("\n")` includes a final empty element `""` if the final line
ends with `\n` (as it should for POSIX-compliant text files), which
then became an extra `"\n"`.

`splitlines()` solves that, but there's a caveat, as it will split
on other types of line breaks too (like `\r`), which may not be
desired.

Fixes psf#526.
@akien-mga
Copy link
Contributor Author

Note that I didn't touch other uses of split("\n") which are not linked to the referenced issue, but contributors here might want to check if those occurrences can trigger a similar EOF issue:

$ rg --fixed-strings 'split("\n"'
black.py
2379:    for index, line in enumerate(prefix.split("\n")):

tests/data/comments2.py
299:        element.split("\n", 1)[0]

tests/data/comments3.py
10:        element.split("\n", 1)[0]

@JelleZijlstra JelleZijlstra merged commit 959848c into psf:master Apr 5, 2020
@JelleZijlstra
Copy link
Collaborator

Thanks! The other cases look harmless (two are in test data and one always ignores an empty line at the end).

@akien-mga akien-mga deleted the fix-diff-newline-at-eof branch April 5, 2020 09:33
harenbrs referenced this pull request in harenbrs/bleck Apr 8, 2020
`split("\n")` includes a final empty element `""` if the final line
ends with `\n` (as it should for POSIX-compliant text files), which
then became an extra `"\n"`.

`splitlines()` solves that, but there's a caveat, as it will split
on other types of line breaks too (like `\r`), which may not be
desired.

Fixes #526.
akien-mga added a commit to akien-mga/godot that referenced this pull request Jul 26, 2020
Until psf/black#1328 makes it in a stable release,
we have to use the latest from Git.

Apply new style fixes done by latest black.
MarcusElg pushed a commit to MarcusElg/godot that referenced this pull request Oct 19, 2020
Until psf/black#1328 makes it in a stable release,
we have to use the latest from Git.

Apply new style fixes done by latest black.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--diff output does not git apply cleanly when the diff context includes EOF
2 participants