Separate GetResourceString method with defaultString value.#51073
Separate GetResourceString method with defaultString value.#51073marek-safar merged 1 commit intodotnet:mainfrom
Conversation
It's not used in Release build and it injects ununsed null value to every callsite
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Is this true? e.g. |
stephentoub
left a comment
There was a problem hiding this comment.
That said, splitting it seems fine. Most assemblies don't need the latter.
| #endif | ||
|
|
||
| internal static string GetResourceString(string resourceKey, string? defaultString = null) | ||
| internal static string GetResourceString(string resourceKey) |
There was a problem hiding this comment.
This creates a difference from its .vb counterpart.
There was a problem hiding this comment.
Does it matter? The only place where it's used is in archived Microsoft.VisualBasic.Core library
There was a problem hiding this comment.
I don't know that it matters, but I wanted to call it out.
cc: @tarekgh
There was a problem hiding this comment.
The only place where it's used is in archived Microsoft.VisualBasic.Core library
that is right. if this change is not affecting Microsoft.VisualBasic.Core, it should be ok. But should we just keep C# and VB match for consistency?
There was a problem hiding this comment.
I'd rather not touch Microsoft.VisualBasic.Core code and I don't think the consistency matters here.
runtime/src/libraries/System.ComponentModel.TypeConverter/src/System/Timers/TimersDescriptionAttribute.cs The value is always null. I intentionally didn't want to mix the clean up with PR runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs That's the only case runtime/src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/Errors/ErrorFacts.cs The assembly is archived so I didn't update the code (it's not worth it) The only cases where it's really used in Release build are StackTrace.cs and System.ComponentModel.Annotations library if that's what you were asking for. |
I was mainly questioning the assertion. |
It's not used in Release build and it injects ununsed null value to every callsite