Use internationally viable formatting for dates used in, for example, file paths#46745
Conversation
|
The "u" format includes colons that are not valid for file paths on Windows. |
|
Could instead use String.Create(IFormatProvider? provider, ref System.Runtime.CompilerServices.DefaultInterpolatedStringHandler handler) with CultureInfo.InvariantCulture. |
I'm trying to figure out how to make this make sense, and I don't, I'm afraid. The IFormatProvider can be CultureInfo.InvariantCulture, but what to use for the DefaultInterpolatedStringHandler? That essentially takes in a format string and some arguments and interpolates them, as the name suggests. There's a format in the current version already, so that would just mean replacing one format with another, and I don't see how that improves things. I'm thinking about just replacing ':' (and perhaps ' ') with '_' in the 'u' format. Is there any reason that wouldn't work? |
|
I meant like |
|
Thanks for the extra color 🙂 That sounds reasonable to me; I want to do some testing to make sure it works as expected, and if so, I can switch to that. |
|
Ok, made a console to check out the difference. This was the result: Will push the change 🙂 |
| protected static string TimeStamp => $"[{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff}]"; | ||
| protected static string TimeStamp => $"[{string.Create(CultureInfo.InvariantCulture, $"Microsoft.NET.Workload_{Environment.ProcessId}_{DateTime.Now:yyyyMMdd_HHmmss_fff}.log")}]"; |
There was a problem hiding this comment.
Isn't this what's written as text to each log entry in the log file? I wouldn't expect the process ID and ".log" there. The nested interpolation looks unnecessarily complex, too.
There was a problem hiding this comment.
For this, $"[{DateTime.Now:u}]" would be safe because the result is not used as part of a file path. However, the u format does not include milliseconds, unlike the current format. Have the milliseconds in the log been useful for tracking down performance problems?
There was a problem hiding this comment.
Hmm. I'm confused here. How does string.Create(CultureInfo.InvariantCulture, apply the culture to the string?
There was a problem hiding this comment.
To clarify, in the past I've used this version of ToString on the DateTime to set it to invariant culture.
date.ToString("yyyyMMdd", CultureInfo.InvariantCulture);There was a problem hiding this comment.
How does string.Create(CultureInfo.InvariantCulture, apply the culture to the string?
An expression like string.Create(CultureInfo.InvariantCulture, $"[{DateTime.Now:yyyyMMdd}]") calls public static string Create(IFormatProvider? provider, [InterpolatedStringHandlerArgument(nameof(provider))] ref DefaultInterpolatedStringHandler handler). The C# compiler generates code to convert the interpolated string $"[{DateTime.Now:yyyyMMdd}]" to a DefaultInterpolatedStringHandler instance. Because of the InterpolatedStringHandlerArgumentAttribute, the value of the IFormatProvider? provider parameter, i.e. CultureInfo.InvariantCulture, is passed to the DefaultInterpolatedStringHandler constructor.
There was a problem hiding this comment.
@KalleOlaviNiemitalo Makes sense. I've only briefly worked with interpolated string handler in the past so I didn't think about it pulling apart things like that. Nice! Thanks for the explanation.
Fixes #46727
Fixes #37932
(@MiYanni commented that it looks similar to the issue I'd been looking at. Turns out 46727 is a dupe, but since I have a PR out already, I'll leave them up and close both via this.)
This replaces an explicit format with "u" for two specific DateTime.ToString instances. Lowercase u is for a sortable, universal date/time pattern:
(https://learn.microsoft.com/dotnet/standard/base-types/standard-date-and-time-format-strings)
I saw a number of other instances in which we currently use an explicit string format like this. I was a little split on whether I should change them all without looking at it too closely, but I can if desired.