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 explain rendering with Windows EOL #14456

Merged
merged 1 commit into from Feb 12, 2022

Conversation

mbovel
Copy link
Member

@mbovel mbovel commented Feb 11, 2022

[test_windows_full]

@@ -254,7 +254,7 @@ trait MessageRendering {
sb.append(EOL).append(newBox())
sb.append(EOL).append(offsetBox).append(" Explanation (enabled by `-explain`)")
sb.append(EOL).append(newBox(soft = true))
dia.msg.explanation.split(EOL).foreach { line =>
dia.msg.explanation.split("\n").foreach { line =>
Copy link
Contributor

Choose a reason for hiding this comment

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

scala> "a\rb\r\nc\nd".split(raw"\R")
val res0: Array[String] = Array(a, b, c, d)

is convenient.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, I didn't know about it. Thanks!

@mbovel
Copy link
Member Author

mbovel commented Feb 11, 2022

@nicolasstucki I found the problem! 🥳 In your last change, you split a string using EOL but its line ends are always encoded with \n even on Windows. So lines do not get the left border there (which was impossible to spot on the CI logs because left borders are anyway stripped there). A quick fix is to use \n instead (or \R as suggested by som-snytt). This PR does that.

Long-term, we might want to use EOL everywhere instead of \n?

@mbovel mbovel marked this pull request as ready for review February 11, 2022 18:30
@som-snytt
Copy link
Contributor

Just when you thought it was safe to strip left borders: #17783

I used to see line ending problems often on cygwin using scala 2. I began to prefer f"%n" (which just came up recently, as someone joked on discord that it looks like foul language). The other use case is stripMargin which winds up as

"""|text""".stripMargin.linesIterator.mkString(EOL)

where you really want a macro interpolator to automate it.

@nicolasstucki
Copy link
Contributor

Long-term, we might want to use EOL everywhere instead of \n?

@mbovel could you open an issue to track and explain the details that need to be changed?

@nicolasstucki nicolasstucki merged commit ddac928 into scala:main Feb 12, 2022
@nicolasstucki nicolasstucki deleted the mb/windows-explain-eol branch February 12, 2022 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants