prevent universal_newlines modifying --draft output

[bennyrowland]

Description

Fixes #524

News fragments are read in binary mode, so on Windows they keep their \r\n newlines. When these are echoed to the console in --draft mode, Python's universal newlines support converts the \n to os.linesep (\r\n), leaving them as \r\r\n. If this output is then read by Python again (in text mode), that same newline support converts both \r and \r\n into \n, causing all newlines to be doubled up (\n\n). This commit echoes the --draft output as utf8 encoded bytes rather than a str to prevent this from happening. This also matches the behaviour of writing to a newsfile in non-draft mode.

@adiroiban please let me know what tests (if any) you think would be appropriate for this change and I will add them. I will also add a news fragment detailing the change if you are minded to accept the change.

Checklist

• Make sure changes are covered by existing or new tests.
• For at least one Python version, make sure local test run is green.
• Create a file in src/towncrier/newsfragments/. Describe your
  change and include important information. Your change will be included in the public release notes.
• Make sure all GitHub Actions checks are green (they are automatically checking all of the above).
• Ensure docs/tutorial.rst is still up-to-date.
• If you add new CLI arguments (or change the meaning of existing ones), make sure docs/cli.rst reflects those changes.
• If you add new configuration options (or change the meaning of existing ones), make sure docs/configuration.rst reflects those changes.

[SmileyChris]

Wouldn't the better thing to do here be just stop opening fragments in binary mode?

In _builder.py:

-            with open(full_filename, "rb") as f:
-                data = f.read().decode("utf8", "replace")
+            with open(full_filename, encoding="utf8", errors="replace") as f:
+                data = f.read()

[bennyrowland]

I can confirm that that would solve my use case, but I assumed that there was some good reason for reading the fragments in binary mode in the first place. I knew that changing the output at this level would not have any side effects on the rest of towncrier, but changing the input might, which is why I proposed this change. If the maintainers are happy with changing the input side of things instead then that would be fine with me.

[SmileyChris]

Well if it passes the test suite then it should be fine 😂
It does seem the better fix to do it right imo

[adiroiban]

Wouldn't the better thing to do here be just stop opening fragments in binary mode?

I think that the fragments should be just text, so is safe to open them in text mode.

As mentioned by Chris, if the change passes the test it should be good enough.

But it's very important to add a new test that would fail with the code from trunk and that is green with the changes from this PR.

Otherwise, in the future, someone else might revert this change...and the tests would still be green and the PR could be approved.

I don't know what test to suggest ... try to get some input similar to what you have on Windows and follow the same steps.

I can see that we run the tests on Windows, so we should be covered.

Fell free to skip the tests when running on Linux.

Thanks

[adiroiban]

I prefer to have a self contained comment, that explains why this is needed without having to search the internet / GitHub :)

I still don't fully understand what went wrong here and what is the best way to fix it..

As mentioned by Chris, I think that a better idea is to fix this inside render_fragments

[adiroiban]

Thanks for the changes.

This needs a release notes fragment and at least one new test, which is executed at least on Windows