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 is corrupting CRLF files #671

Closed
jchv opened this issue Feb 11, 2018 · 2 comments
Closed

isort is corrupting CRLF files #671

jchv opened this issue Feb 11, 2018 · 2 comments

Comments

@jchv
Copy link
Contributor

jchv commented Feb 11, 2018

With Python 3.6.3 and isort 4.3.3, my CRLF files are being corrupted. Every line ends with CR CR LF instead of CR LF; a lot of software treats this sequence differently, but it usually manifests as either two line endings or one line ending and an excess CR. Needless to say, this is painful once it occurs, leaving a lot of a codebase damaged. :)

The problem is really obvious now that I look, so I'll submit a PR. Basically:

1. CRLF is applied upon joining the lines.

        self.output = self.line_separator.join(self.out_lines)

2. CRLF is applied again upon saving the file. The newline argument seems to blindly replace all instances of \n with whatever you stick in it, even though the lines already end in \r\n.

            with io.open(self.file_path, encoding=self.file_encoding, mode='w', newline=self.line_separator) as output_file:
                print("Fixing {0}".format(self.file_path))
                output_file.write(self.output)

Kind of off-topic: anyone have any idea why this is broken now? It seems really unlikely to me that it literally never worked, and yet since the PR that was supposed to fix line endings, it doesn't seem the code has changed dramatically. My best guess is that Python's behavior for newline used to have heuristics to prevent this, but that seems asinine...

timothycrosley added a commit that referenced this issue Feb 12, 2018
Do not apply line endings twice. (#671)
timothycrosley added a commit that referenced this issue Feb 12, 2018
@timothycrosley
Copy link
Member

I'm not sure why this broke, or why it ever worked, but I've taken your fix and made a hotfix release with it - I appreciate you finding and fixing the issue! I've added a test to hopefully ensure it won't happen again: 682c2ea

@jchv
Copy link
Contributor Author

jchv commented Feb 12, 2018

👍 I didn't even think to make a test, but the care is appreciated.

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

No branches or pull requests

2 participants