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 newlines in config files #98

Merged
merged 4 commits into from Jan 18, 2020

Conversation

kyluca
Copy link
Contributor

@kyluca kyluca commented Oct 31, 2019

This change should still be sufficient to close #58.

Heavily inspired by the previous patches and PR jtrakk#1, however it appears the original PR wasn't merged in upstream?

@kyluca
Copy link
Contributor Author

kyluca commented Oct 31, 2019

Build failed with a temporary network issue:

ProtocolError: ("Connection broken: error(104, 'Connection reset by peer')", error(104, 'Connection reset by peer'))

Will rebuild

@arielnmz
Copy link

Nice, thank you so much

@florisla
Copy link
Collaborator

We also have to take care not to change the newline style of the file.

I think that currently this PR will produce a weird result for Linux users who use <carriage return><newline> as line endings in the config file: the very last line ending will miss the carriage return.

Same goes for Windows users who have only <newline> as line ending; the last line will include a carriage return.

See merge request #59 which tries to keep the line endings consistent.

I think it would make sense to add a test like test_retain_newline which specifically tests that the final newline of the file is of the right type.

@kyluca
Copy link
Contributor Author

kyluca commented Nov 1, 2019

Thanks for the suggestion, have updated the tests to explicitly check the final line ending occurs once (not twice) and is of the right type. Also confirmed locally that

  • the test failed without the code change (with two line endings at the end of the file)
  • and passed with the code change (with one line ending at the end of the file).

The code is currently making use of Python's universal newline conversion handling when using open in text mode.

  • During the read operation, we take note of the original line endings and use the default for newline=None so Python converts them to \n for us.
  • At the other end during the write operation, we pass the original line endings back with newline=config_newlines to convert \n in the content to \r\n or whatever the original file format was (platform independent).

Excerpt from the docs: https://docs.python.org/3/library/functions.html#open

newline controls how universal newlines mode works (it only applies to text mode). It can be None, '', '\n', '\r', and '\r\n'. It works as follows:

  • When reading input from the stream, if newline is None, universal newlines mode is enabled. Lines in the input can end in '\n', '\r', or '\r\n', and these are translated into '\n' before being returned to the caller. If it is '', universal newlines mode is enabled, but line endings are returned to the caller untranslated. If it has any of the other legal values, input lines are only terminated by the given string, and the line ending is returned to the caller untranslated.
  • When writing output to the stream, if newline is None, any '\n' characters written are translated to the system default line separator, os.linesep. If newline is '' or '\n', no translation takes place. If newline is any of the other legal values, any '\n' characters written are translated to the given string.

@florisla
Copy link
Collaborator

florisla commented Jan 8, 2020

Look good to me!

I'm in favor of merging this.

@florisla
Copy link
Collaborator

@ekohl Can you look into this one too?

@c4urself c4urself merged commit 4f9110d into c4urself:master Jan 18, 2020
@avilaton
Copy link

Thanks

@florisla florisla added this to the New in master milestone Jan 19, 2020
martinm82 pushed a commit to martinm82/bump2version that referenced this pull request Nov 17, 2020
* Fix newlines in config files

* Update tests which previously expected two blank lines, now expect only one

* Update test_retain_newline to explicitly check for a single newline of the right type at the end of the config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bumpversion command adds a newline in bumpversion.cfg
5 participants