-
Notifications
You must be signed in to change notification settings - Fork 223
Remove color parsing from Coreclr on Linux and Mac #1724
Conversation
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 don't think parse is the right term here. Maybe useColors or something on those lines would be more appropriate
|
Looks good overall, except the places where I commented. |
20698ee to
7ffe15e
Compare
|
@victorhurdugaci |
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.
Naming convention for fields: _camelCaseName
7ffe15e to
bda2b60
Compare
|
looks good. |
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.
Designwise, I don't think we should cache _output and _error here. These are static field and the value depends on runtimeEnv. It will probably work fine now if runtimeEnv does not change but if for whatever reason GetOutput() was called multiple times with different runtimeEnv value the returned value for the second call would be wrong because it would use runtimeEnv from the first call. I think the GetOutput and GetError should just create and return the value and the code that invokes these methods should take care of caching the returned instance if it makes sense.
bda2b60 to
50b1a8a
Compare
|
@davidfowl |
6c8c310 to
32e295c
Compare
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.
Format: space after if
|
|
c2cbea5 to
1a2460d
Compare
|
Agree with @davidfowl that |
|
He suggested |
|
Why is the build failing? |
a88b6c9 to
9998e35
Compare
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 name doesn't feel right. You are using ansiCodes either way, just processing them differently. It is more like ansiCodePassthrough, but given that nobody knows what that means and this is internal it doesn't worry me too much.
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 changes the behaviour of this does it not? Previously everyone who called this would've gotten the same instance. Now they will all get a different one. Not sure it matters, but it is a thing. @davidfowl?
9998e35 to
c73a8db
Compare
|
|
#1708