Fix coloring of info messages in isoltest #14580
Conversation
cameel
force-pushed
the
fix-isoltest-info-color
branch
2 times, most recently
from
9d2c376 to
6c1ed8c
Compare
| { | ||
| case Error::Severity::Error: return RED_BACKGROUND; | ||
| case Error::Severity::Warning: return ORANGE_BACKGROUND_256; | ||
| case Error::Severity::Info: return GRAY_BACKGROUND; |
There was a problem hiding this comment.
LGTM. Just one question, wouldn't this allows to display the text in white over gray background?
There was a problem hiding this comment.
Wait a second... we do have colored backgrounds for these :-D? That's weird - I mean, that's generally horrible, isn't it? But also I've never seen that in any output myself, even though the escape actually works in my terminal...
There was a problem hiding this comment.
LGTM. Just one question, wouldn't this allows to display the text in white over gray background?
White text actually looks fine on this background. That's why I chose this color.
Wait a second... we do have colored backgrounds for these :-D? That's weird - I mean, that's generally horrible, isn't it? But also I've never seen that in any output myself, even though the escape actually works in my terminal...
We use it in tests for highlighting parts of the source matching a particular error/warning/info. I actually find it pretty convenient - having to decode character offsets from source locations in your head sounds more horrible :) For example you can see it in DebugWarner output in new analysis when a test does not pass. It's not used for compilation errors by the compiler.
Since it's not used outside of tests, I could put it somewhere in test utils, but putting it together with foreground colors seemed like a better choice to me. If I ever needed it and were ever looking for it, I'd probably look here (or in AnsiColorized.h, but it did not seem right to make that file depend on the Error definition).
There was a problem hiding this comment.
Right. Just mentioned because I remembered this issue: #14378. So I guess this PR would also fix that, right? At least in some tests I did here using a white background, everything seems to work fine.
There was a problem hiding this comment.
I actually missed the "in isoltest" part of this PR (not sure how :-)) and thought it was about regular compiler output, that's why I was confused :-).
So just to confirm, this is not meant to change colored solc output, but just to affect isoltest? (I could probably also read the code for an answer :-))
There was a problem hiding this comment.
It uses errorColored which in turn is used by printExceptionInformation and by the CommandLineInterface to print compiler errors in the CLI. This was why I initially thought it could be solving the issue that I mentioned above (i.e. not only affecting isoltests), but it seems that the code doesn't really change anything in the errorColored logic.
There was a problem hiding this comment.
So just to confirm, this is not meant to change colored solc output, but just to affect isoltest? (I could probably also read the code for an answer :-))
Exactly. There are no behavior changes outside of tests. The non-test code is only refactored.
So I guess this PR would also fix that, right?
No, at least not in a significant way. The issue is not limited to test output.
BTW, not sure what the "fix" would be there. I'd assume that the terminal that changes colors of various things would do that by remapping the meaning of these colors to something else. Or are there some "themable"/relative color codes that we should be using to play nice with such terminals?
There was a problem hiding this comment.
BTW, not sure what the "fix" would be there. I'd assume that the terminal that changes colors of various things would do that by remapping the meaning of these colors to something else. Or are there some "themable"/relative color codes that we should be using to play nice with such terminals?
I'm not sure either. I know that on GNU/Linux you could use tput to query for terminal-dependent information, but I don't know if a solution based on that, or on ncurses, would be reliable and cross-platform. On GNU/Linux you can do tput colors to get the maximum number of colors supported by the terminal according with the terminfo database, but it may not really reflect the current terminal capabilities. And I'm not sure you can query for specific colors either. But if there is a way to reliably detect the background color and set the default as the opposite color, then we would have a fix for it.
cameel
force-pushed
the
fix-isoltest-info-color
branch
from
6c1ed8c to
8eed91a
Compare
cameel
added
the
has dependencies
cameel
changed the base branch from
develop
to
ci-stop-using-deprecated-macos-large
cameel
removed
the
has dependencies
cameel
force-pushed
the
fix-isoltest-info-color
branch
2 times, most recently
from
5518ebc to
daca358
Compare
cameel
force-pushed
the
fix-isoltest-info-color
branch
3 times, most recently
from
d224c61 to
4245789
Compare
|
Although I approved, I just noticed there are some conflicts. |
|
Yeah, just a rebase and it is fine for me as well :) |
cameel
force-pushed
the
fix-isoltest-info-color
branch
from
4245789 to
a017797
Compare
|
Rebased. |
cameel
force-pushed
the
fix-isoltest-info-color
branch
from
a017797 to
0da44ca
Compare
Depends on #14585.Merged.Isoltest prints "info" messages colored as errors. Apparently we missed that when we introduced the new level, probably because we still have very few such messages.
Now that #14510 is printing a lot of them, it's getting really annoying. This PR fix that and does some small refactors to prevent this getting out of sync so easily in the future.