Made telemetry data constants true constants #3416
Conversation
There was a problem hiding this comment.
LGTM if we are ok we introducing a "potential" breaking change. We don't expect people to use these but that's public and not currently readonly.
Also, I don't know if we can end up in a situation were we would benefit from having static readonly instead of const (I am thinking about some case were we would have different versions loaded at the same time with the string value being different). cc @nohwnd
There was a problem hiding this comment.
LGTM if we are ok we introducing a "potential" breaking change. We don't expect people to use these but that's public and not currently readonly.
Also, I don't know if we can end up in a situation were we would benefit from having static readonly instead of const (I am thinking about some case were we would have different versions loaded at the same time with the string value being different). cc @nohwnd
|
If we're potentially breaking why not put internal? |
|
@cvpoienaru Do you have any motivation for this change other than not liking the current syntax? Constants will get compiled into the consuming dll. Readonly static strings won't. |
|
@nohwnd The problem with current solution is the missing |
|
@nohwnd, @Evangelink pretty much covered the reason for this change. This came up during the telemetry improvements PR review when it was pointed out that it'd be best to make our intention even clearer by making the fields true constants. Whether that means CC: @MarcoRossignoli |
|
The change to internal should be fine .Common. is shipped with runner and host, and uwp provider also does not load it dynamically. So on binary level it should not break anyone. |
Made telemetry data constants true constants