Fix #4405 REPL highlight for StringLiteral #4531
Conversation
|
I sign the CLA |
Blaisorblade
changed the title
|
Thanks @fabianpage, CLA signed. I'll leave the actual review to somebody else. |
|
@fabianpage Can you please add test cases to s"Hello"
s"Hello $name!"
raw"Hello" |
|
@allanrenucci I think these 3 tests are enough or do you think it need's some more? |
| @@ -50,6 +50,9 @@ class SyntaxHighlightingTests { | |||
| def strings = { | |||
| // For some reason we currently use literal color for string | |||
| test("\"Hello\"", "<L|\"Hello\">") | |||
| test("s\"Hello\"", "s<L|\"Hello\">") | |||
| test("s\"Hello $name\"", "s<L|\"Hello <V|$name<L|\">") | |||
| test("raw\"Hello\"", "raw<L|\"Hello\">") | |||
There was a problem hiding this comment.
Let's add one more with triple quotes: s"""Hello"""
| case _ => false | ||
| } | ||
|
|
||
| def tryInterpolatorPrefix(n: Char, remaining: Stream[Char]): Option[String] = { |
There was a problem hiding this comment.
I don't fully understand what you are doing below. Could you explain in detail?
The previous solution didn't work because it assumed that the interpolator prefix was on length 1. So we just need to consume characters as long as they are valid prefixes. Something like:
val (prefix, after) = remaining.span(interpolationPrefixes.contains)
if (after.startsWith("\"")) {
...There was a problem hiding this comment.
The problem was that it also didn't supported string literals with symbols in it. something like a_+""
|
@allanrenucci back from Berlin. Should i continue working on this PR or will #4537 be merged soon and my change is not needed any more? |
|
Not sure when #4537 will be merged. But yes, the fix in it is more generic and it will replace all the actual code that does syntax highlighting. |
|
@allanrenucci Then i will try to finish this PR. Should i try to change the existing code to only support longer interpreter suffix? |
Sounds good! Then we can get this PR merged before #4537 |
|
@allanrenucci What do you think? I think this way there are only minimal changes. |
| appendString('"', next == "\"\"\"") | ||
| val (prefix, after) = remaining.span(interpolationPrefixes.contains) | ||
| if (after.startsWith("\"")) { | ||
| newBuf ++= (n +: prefix) |
There was a problem hiding this comment.
This will create an intermediate collection, prefer:
newBuf += n ++= prefix
|
Thanks @fabianpage! 🎉 |
Fix #4405.