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

Always use "binary" mode when opening files on Windows. #2811

Merged
merged 1 commit into from
Jun 27, 2018

Conversation

chalcolith
Copy link
Member

By default Windows will translate \n to \r\n when writing to files, and vice versa when reading. This interacts poorly with seeking in the file first (#2707). This change makes opening files via the files package always use "binary" mode, which will not do any translation.

By default Windows will translate `\n` to `\r\n` when writing to files, and vice versa when reading.  This interacts poorly with seeking in the file first (ponylang#2707).  This change makes opening files via the `files` package always use "binary" mode, which will not do any translation.
@chalcolith chalcolith added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Jun 27, 2018
@mfelsche
Copy link
Contributor

Very nice! Do you think it makes sense to have a ponytest testcase for this? Something along the lines of writing something with libebreaks to a file then reading it and assert it is the same.

@chalcolith
Copy link
Member Author

That wouldn't really prove anything, since Windows does the translation both for reading and writing. I think that the existing test that is failing in #2707 is sufficient to test this.

Copy link
Contributor

@mfelsche mfelsche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case there is nothing left for me but to approve this one.

@sylvanc sylvanc merged commit c7ca4cb into ponylang:master Jun 27, 2018
ponylang-main added a commit that referenced this pull request Jun 27, 2018
@chalcolith chalcolith deleted the windows_binary_mode branch June 27, 2018 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants