bpo-11594: Ensure line-endings are respected when using 2to3

[aaronang]

https://bugs.python.org/issue11594

[jaraco]

Looks good to me!

[jaraco]

Can you add a news file also?

[aaronang]

@jaraco Done 👍

[ambv]

Would be nicer if you extracted lines 210-221 to a separate function that both check_file_refactoring() and refactor_file() would reuse. Now there's copypasta.

[ambv]

Wow, that's hilarious. We've had a \r\n file but something along the way wiped the newlines O_O

[aaronang]

@ambv Thank you for taking the time for reviewing this patch. I didn't refactor it exactly the way you told me to because I felt it was too much "extracting the similarities just for the sake of keeping the code DRY". Instead, I extracted certain functionality, "initializing the test file" and "reading the test file", into separate functions init_test_file and read_file which made more sense to me. Looking forward to your feedback 🙂

[ambv]

Much better. Thanks!

[miss-islington]

Thanks @aaronang for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

[bedevere-bot]

GH-6515 is a backport of this pull request to the 3.7 branch.

[miss-islington]

Sorry, @aaronang and @ambv, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker c127a86e1862df88ec6f9d15b79c627fc616766e 3.6

[blah238]

@aaronang Did this not make the 3.7.0 release? I'm seeing my line endings being changed from CRLF to CRCRLF (yikes!) on Windows. Or perhaps that's a different issue?

[jaraco]

According to the news file, this PR was included in 3.7.0b4. Sounds like you may have identified a new issue.

[blah238]

@jaraco Good to know. All tests passed on my Windows PC. Could you add a test case for CRLF files on Windows? I don't know if the automated tests are run against Windows but I could run it manually if needed.

[jaraco]

@aaronang I think I see where things went wrong. In adding newline='' during the read phase, that caused the newlines to be untranslated. But then in the write phase, we left newline=None (the default), which causes \n to be translated to \r\n. As a result, CRLF -> CRCRLF (ref)

I think the patch also needs something like:

cpython master $ git diff
diff --git a/Lib/lib2to3/refactor.py b/Lib/lib2to3/refactor.py
index 7c4e064997..7841b99a5c 100644
--- a/Lib/lib2to3/refactor.py
+++ b/Lib/lib2to3/refactor.py
@@ -514,7 +514,7 @@ class RefactoringTool(object):
         set.
         """
         try:
-            fp = io.open(filename, "w", encoding=encoding)
+            fp = io.open(filename, "w", encoding=encoding, newline='')
         except OSError as err:
             self.log_error("Can't create %s: %s", filename, err)
             return
diff --git a/Lib/lib2to3/tests/test_refactor.py b/Lib/lib2to3/tests/test_refactor.py
index f3059a9311..9e3b8fbb90 100644
--- a/Lib/lib2to3/tests/test_refactor.py
+++ b/Lib/lib2to3/tests/test_refactor.py
@@ -300,6 +300,7 @@ from __future__ import print_function"""
         old, new = self.refactor_file(fn)
         self.assertIn(b"\r\n", old)
         self.assertIn(b"\r\n", new)
+        self.assertNotIn(b"\r\r\n", new)

     def test_refactor_docstring(self):
         rt = self.rt()

[aaronang]

I am sorry for the late reply. I am looking into it now.

[aaronang]

@blah238 Could you apply this patch to the master:

diff --git a/Lib/lib2to3/tests/test_refactor.py b/Lib/lib2to3/tests/test_refactor.py
index f3059a9311..9e3b8fbb90 100644
--- a/Lib/lib2to3/tests/test_refactor.py
+++ b/Lib/lib2to3/tests/test_refactor.py
@@ -300,6 +300,7 @@ from __future__ import print_function"""
         old, new = self.refactor_file(fn)
         self.assertIn(b"\r\n", old)
         self.assertIn(b"\r\n", new)
+        self.assertNotIn(b"\r\r\n", new)

     def test_refactor_docstring(self):
         rt = self.rt()

And run:

$ ./python.exe -m unittest -v lib2to3.tests.test_refactor.TestRefactoringTool.test_crlf_unchanged

For me this doesn't fail locally.

[jaraco]

@aaronang to be clear, when the test didn't fail, did you run it on Windows? I didn't run the test suite, but I did verify the behavior on a Windows machine.

[aaronang]

@jaraco I am sorry for being unclear. I don't have a Windows machine so I couldn't test whether the test with the patch (that you proposed) fails or not on Windows.

[aaronang]

@jaraco I am sorry for asking this but could you test out the patch that you proposed to see if it fixes the problem? I am not really capable of fixing the problem because I don't have a Windows machine available. I am a bit embarrassed for asking this 😓

[jaraco]

I’m pretty sure this is the right fix. I’ll submit a PR with just the test update to demonstrate the failure then apply the fix.

[aaronang]

@jaraco I really wanted to help out but don't have a Windows machine available. Thanks a lot!

[jaraco]

@blah238 - The fix should come in 3.7.1, but feel free to grab the latest refactor.py from the Python 3.7 branch and replace it in your installation.

[blah238]

Great to hear, thanks @jaraco and @aaronang! Much appreciated.