Fix Logging formatting with multiple collections #106213
Conversation
|
@stephentoub @ericstj this is a regression in .NET 8.0. I believe this is meeting the servicing bar and we should port the fix there. |
| !TryFormatArgumentIfNullOrEnumerable(arg1, ref arg1String) ? | ||
| string.Format(CultureInfo.InvariantCulture, _format, arg0, arg1) : | ||
| string.Format(CultureInfo.InvariantCulture, _format, arg0String ?? arg0, arg1String ?? arg1); | ||
| TryFormatArgumentIfNullOrEnumerable(arg0, ref arg0String) | TryFormatArgumentIfNullOrEnumerable(arg1, ref arg1String) ? |
There was a problem hiding this comment.
Should we make TryFormatArgumentIfNullOrEnumerable set stringValue to null when it returns false just to be explicit? I wonder if there is really any reason to check the return value from the Try method at all.
There was a problem hiding this comment.
Should we make TryFormatArgumentIfNullOrEnumerable set stringValue to null when it returns false just to be explicit?
I think it is written this way to avoid forcing initializing the stringValue inside the method. But we initlaize anyway the local variables before calling the Try methods. We can apply your suggestion to be consistent with other Try patterns.
I wonder if there is really any reason to check the return value from the Try method at all.
The only benefit of the checking is to avoid rechecking the args again when calling string.Format. I am not seeing is a big deal eitherway.
Fixes #103338