Fix Windows-style newlines #428
Open
+343
−200
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There still are a few XXX markers where it's clear from the code that it should deal with \r, but I couldn't pin-point a test case to exercise it yet.
This mainly adds a re-run of all reference tests with Unix newlines replaced with Windows ones. Plus, fixes for a bazillion of problems that this test uncovered. Node now contains a ContainsWindowsNewlines field, which is set to true when the contents of the node (either in .Literal or in .content) include text having '\r' characters. This is then checked at render time for replacement back to Unix newlines.
After introducing a second run of reference tests (with Windows-style newlines), tests hit the 10m deadline very hard. Bump the timeout by a lot to see how much is actually needed. The right fix is probably to drop the race detector, since we don't have any parallelism and it doesn't buy us much, only burns the CPU cycles, but that should be addressed separately.
|
Any plans to merge this? |
Collaborator
Author
Not yet... The problem is that this PR is very much incomplete. I have a branch with much more fixes on top of this, but even they are not sufficient yet, there are a bunch of tests not passing yet. |
|
Any progress on this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There still are a few XXX markers where it's clear from the code that it should deal with \r, but I couldn't pin-point a test case to exercise it yet.
Also, I'm planning to add a test runner that will translate all the
\ns to\r\ns and re-run the tests.Fixes #394 and #423.