-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #3614: Do not allow the SyntaxFormatter to recolor code #3616
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
Conversation
| hb.toString | ||
| case str: String if ctx.settings.color.value != "never" => | ||
| new String(SyntaxHighlighting(str).toArray) | ||
| if (str.matches(".*\u001b\\[.*?m.*")) str // was already colored by some other printer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very expensive. Error messages are colored because of the hl string interpolator. The interpolator call show on its arguments. Can we instead disable syntax highlighting when calling show for the argument of the interpolator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redefining hl as:
def hl(args: Any*)(implicit ctx: Context): String = {
val nestedCtx = ctx.fresh.setSetting(ctx.settings. color, true)
new SyntaxFormatter(sc).assemble(args)(nestedCtx).stripMargin
)should work. I am wondering how expensive is ctx.fresh though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might work if the argument in args is still of type Text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point it is too late. I created instead an AlreadyColored that fixes the issue for each specific case. It should be added to any printed tree that is already colored.
7670223 to
610d37f
Compare
610d37f to
23e7026
Compare
allanrenucci
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, LGTM
| new String(SyntaxHighlighting(str).toArray) | ||
| case _ => super.showArg(arg) | ||
| case x :: xs => | ||
| showArg(x) + (if (xs.isEmpty) "" else ", " + showArg(xs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this special case. This seems completely arbitrary to special case List.
If a user want to pretty print a List like this, then he should do it himself.
23e7026 to
450c0d8
Compare
No description provided.