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 line separator issues #1887

Merged
merged 4 commits into from May 6, 2021
Merged

Conversation

rnett
Copy link
Contributor

@rnett rnett commented Apr 24, 2021

Simple fix for #1884 and #1883, just replaces CRLF line separators with LF before markdown parsing. Ideally this should be fixed in the parser, but this works for now.

Also adds a test for this.

For GitHub:
Fixes #1883
Fixes #1884

@rnett
Copy link
Contributor Author

rnett commented Apr 27, 2021

Tests are failing because they were in the wrong spot, parseModuleAndPackageDocFragment does not use the markdown parser. parseModuleAndPackageDocFragment supports CRLF fine, but leaves them in the output. Is that intentional/a good idea, or should I switch those to LF as well (it won't line up with the markdown parser unless I do)?

@@ -37,7 +53,7 @@ open class MarkdownParser(
return visitNode(markdownAstRoot)
}

override fun preparse(text: String) = text
override fun preparse(text: String) = text.replace("\r\n", "\n").replace("\r", "\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a bad feeling about that. What would happen if somebody would add \r\n in their documentation? Or any other escaped version of those characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume (unless you're doing some kind of preprocessing somewhere else) that they won't be unescaped, i.e. a "\n" in the markdown document will be parsed as ['\', 'n'] i.e. "\\n", not ['\n']. I'll add this to the tests.

@MarcinAman MarcinAman merged commit 326048c into Kotlin:master May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants