Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing DateTimeFormatInfo.TimeSeparator on some cultures doesn't work because DateTimeFormatInfo's time patterns doesn't use ':' #77037

Closed
cmeeren opened this issue Oct 14, 2022 · 31 comments

Comments

@cmeeren
Copy link

cmeeren commented Oct 14, 2022

Description

After switching to ICU (I assume), the time separator (DateTimeFormatInfo.TimeSeparator) for some cultures, such as Norwegian (e.g. nb-NO), got changed from : to .. That's fine.

However, it seems like the following patterns were also changed to use ., such as:

  • DateTimeFormatInfo.ShortTimePattern (HH.mm)
  • DateTimeFormatInfo.LongTimePattern (HH.mm.ss)
  • DateTimeFormatInfo.FullDateTimePattern (HH.mm.ss)

This should not have been done: As indicated by the official docs, if the patterns use :, it will be automatically replaced with TimeSeparator. Therefore, the patterns should use :.

Note that this seems to be environment-specific. For example, I have only managed to reproduce this in Azure App Service (.NET 6). In particular, I have not managed to reproduce this locally, where I get : in these patterns even if my regional settings are set to use . as the time separator.

Reproduction Steps

let ci = CultureInfo.GetCultureInfo("nb-NO")
printfn $"{ci.DateTimeFormat.ShortTimePattern}"

Expected behavior

Should print HH:mm.

Actual behavior

Prints HH.mm (on some environments, as mentioned above).

Regression?

No response

Known Workarounds

If changing the TimeSeparator, remember to manually update all relevant time patterns to use : so that TimeSeparator is used.

Configuration

  • Azure App Service for Windows with .NET 6 x64

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 14, 2022
@ghost
Copy link

ghost commented Oct 14, 2022

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

I assume that after switching to ICU, the time separator (DateTimeFormatInfo.TimeSeparator) for some cultures, such as Norwegian (e.g. nb-NO), got changed from : to .. That's fine.

However, the following patterns were also changed to use ., such as:

  • DateTimeFormatInfo.ShortTimePattern
  • DateTimeFormatInfo.LongTimePattern
  • DateTimeFormatInfo.FullDateTimePattern

This should not have been done: As indicated by the official docs, if the patterns use :, it will be automatically replaced with . the patterns should use : so that they automatically use the configured TimeSeparator. Otherwise,

Note that this seems to be environment-specific. For example, I have only managed to reproduce this in Azure App Service (.NET 6). In particular, I have not managed to reproduce this locally, where I get : in these patterns even if my regional settings are set to use . as the time separator.

Reproduction Steps

let ci = CultureInfo.GetCultureInfo("nb-NO")
printfn $"{ci.DateTimeFormat.ShortTimePattern}"

Expected behavior

Should print HH:mm.

Actual behavior

Prints HH.mm (on some environments, as mentioned above).

Regression?

No response

Known Workarounds

If changing the TimeSeparator, remember to manually update all relevant time patterns to use : so that TimeSeparator is used.

Configuration

  • Azure App Service for Windows with .NET 6 x64

Other information

No response

Author: cmeeren
Assignees: -
Labels:

area-System.Globalization

Milestone: -

@Clockwork-Muse
Copy link
Contributor

If changing the TimeSeparator, remember to manually update all relevant time patterns to use : so that TimeSeparator is used.

Generally, you shouldn't be doing this, because normally programs should be relying on the data given to them by the OS for these purposes, and not inspecting the results. Is there a reason you're modifying the separator?

In particular, I have not managed to reproduce this locally

If you're running on Windows, you're probably still hitting NLS, and using legacy data. See this page about the switch.

Therefore, the patterns should use :

Two more sentences later in that remarks section:

We recommend that you set the time separator in short or long time patterns to an exact string instead of using the time separator placeholder.

... so having the data that way would presumptively be the "correct" way to do it. That it "works" in the documentation sample is almost certainly due to US formats using :, and not necessarily as the intended use case. Note that TimeSeparator is primarily intended to be used when custom format strings are used (eg dateTimeValue.ToString("HH:mm:ss.ff K")), rather than any of the standard format strings. That is, instead of doing:

let ci = CultureInfo.GetCultureInfo("nb-NO")
printfn $"{dateTimeValue.ToString(ci.DateTimeFormat.ShortTimePattern)}"

you should instead by doing something like:

let ci = CultureInfo.GetCultureInfo("nb-NO")
let message: FormattableString = $"{dateTimeValue:t}"
printfn message.ToString(ci)

(I dunno if F# actually supports that, but that's the idea)

@cmeeren
Copy link
Author

cmeeren commented Oct 14, 2022

If you're running on Windows, you're probably still hitting NLS, and using legacy data. See this page about the switch.

You are correct; my local machine uses NLS.

Is there a reason you're modifying the separator?

Since the 1970's, official rules in Norway mandated . as the time separator. But in the digital age, : has became increasingly common. In 2014, : was officially accepted alongside .. (Anecdotally, I was born in '88 and can only ever remember : being in common usage, at least digitally.)

Here is an official source in Norwegian (nynorsk) from 2014.

Some of it translated:

According to the international standard NS-ISO 8601, colon shall be used as a separator, and that influences how many people write times on computers. Colon now becomes an alternative in the official [Norwegian] rules.

One concrete problem in our current use-case is that in the context the timestamp is used, using . makes it look like a date, which is confusing for users.

Note that TimeSeparator is primarily intended to be used when custom format strings [... rest of comment]

Sure, I just wanted to highlight that it was the actual value of ShortTimePattern I was talking about. (Though admittedly, that is a kind of X/Y problem; my core issue is that I want to change TimeSeparator and get the expected results when formatting times, no matter the initial value of TimeSeparator).


My general point is that I would expect that setting TimeSeparator to a different value also changes the output when I format using ShortTimePattern etc. (usually using t and similar, not ShortTimePattern directly).

One argument for this is that the API is more self-explanatory: It's one place to set the time separator. It makes for a more intuitive and less confusing API. I expect (rightly so, I would say) that when I set TimeSeparator, I get the chosen value when using all of the standard format specifiers. This is currently not the case. I also don't see a compelling argument to the contrary.

(I interpret your comment to say that this behavior is incidental, not intended, and that one should expect to have to adjust the time patterns like ShortTimePattern in addition to TimeSeparator. I disagree. It is confusing. Furthermore, you say that TimeSeparator is primarily intended to be used when using custom format strings. I don't understand that. If using custom format strings, you control the time separator yourself and do not need to rely on TimeSeparator.)

Another argument is that there is currently a discrepancy here based on the initial value of TimeSeparator (assuming that this initial value is also what's used in the standard time patterns like ShortTimePattern etc., which seems to be the case for the Norwegian culture on ICU):

  • For cultures with : (including Norwegian on NLS), the standard time patterns contain :, and therefore are sensitive to changes in TimeSeparator. For example, on my machine where TimeSeparator is :, I can change TimeSeparator to . (and nothing else) and all times are now formatted using ..
  • For cultures with . (including Norwegian on ICU), the standard time patterns contain ., and therefore are not sensitive to changes in TimeSeparator.

TL;DR

I can write a test that fetches the nb-NO culture (possibly relevant for other cultures, too), changes TimeSeparator, and verifies that when I format using t I get the chosen time separator. If I run this test on my machine (NLS), where the format patterns use :, it succeeds. If I run it on Azure App Service (ICU), where the format patterns use ., it fails.

I know that NLS and ICU brings some changes, but I can't see how this particular behavior is intended, and certainly not desired, for reasons detailed above.

@Clockwork-Muse
Copy link
Contributor

If using custom format strings, you control the time separator yourself and do not need to rely on TimeSeparator

The intention is that you write dateTimeValue.ToString("HH:mm"), and then it replaces the colon with the culture-specific time separator in the output string. It's to prevent you from needing to worry about what the actual character is, and manually constructing the format string during each call.

One argument for this is that the API is more self-explanatory: It's one place to set the time separator.

The documentation for TimeSeparator explicitly states that it's determined from the short time format.... So this is sort-og backwards.

My general point is that I would expect that setting TimeSeparator to a different value also changes the output when I format using ShortTimePattern etc. (usually using t and similar, not ShortTimePattern directly).

This assumes that the patterns are using the separator, which isn't actually a guarantee (other than ShortTimePattern itself, I suppose).

Another argument is that there is currently a discrepancy here based on the initial value of TimeSeparator...

This is potentially interesting, in that it is likely to cause unwanted results. @tarekgh thoughts here?

I can write a test that fetches ...

You can write the test, but depending on deployment target you shouldn't deploy these changes. More specifically:

  • For client applications, it should pick up formats from their machine (which may not be using Norwegian, or may have other customizations).
  • For web applications, it's possible to format dates/times via JS routines into the locale format. This would require a little more work if you're using blazor, but would at least remove the need to distribute the ICU data with your app.
  • For server applications, ideally you shouldn't be dealing with localized values at all.

@tarekgh
Copy link
Member

tarekgh commented Oct 14, 2022

@cmeeren thanks for your report.

  • We never intended to support changing the time separator will change all time patterns for that culture. I don't even think it is the right thing to do. I don't even think you need to override the time separator. If you want to format any time the way you like, use custom formats. If you want to format the time consistently across your app, override the default short pattern.
  • I want to emphasize, exposing the time separator in first place was wrong. There are some cultures that use time patterns which cannot have single time separators. e.g. culture using pattern like hh'h' mm'm' ss's'. What does the time separator mean here? and what does overriding the time separator even mean?
  • The behavior you are requesting can make sense in your scenario but can be a breaking to other scenarios. As users want to override the time separator for custom formatting and avoid touching the original culture patterns.
  • I do not see the discrepancy argument is valid one. We have generic rule : represent the culture time separator placeholder. This rule works consistently and there is no confusion here. : is the only character that can be replaced by the actual culture time separator. Cannot assume using other character like . is going to behave the same.
  • Last, you can handle your scenario by overriding the time separator and the time patterns too the way you like. You have full control over that. Why is this not good enough for you?

In summary, I don't think we can support your request as it will be a very breaking change and not desirable in all scenarios. By that, I am closing this issue but feel free to send any question we can help with.

@tarekgh tarekgh closed this as completed Oct 14, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Oct 14, 2022
@cmeeren
Copy link
Author

cmeeren commented Oct 15, 2022

Thank you both for excellent clarifications. TL;DR: I now understand the issue much better, and I believe a single remark added to the TimeSeparator documentation could have avoided all of this.


For server applications, ideally you shouldn't be dealing with localized values at all.

That is not a general truth. I am using this in two server systems. One sends notification emails, that need to include localized values, such as datetimes. The other produces PDF reports that similarly include localized values.

Furthermore, our system allows the users to specify a language (among currently just two, in the future not many more), but not more detailed times formats or similar. The formatting is performed using a culture based on the user's configured language. As mentioned in a previous comment, for Norwegian, we'd like to use : as the time separator, as it was with NLS (used in local testing), but not ICU (seemingly used on Azure App Service).

If you want to format the time consistently across your app, override the default short pattern.

That is what I ended up doing (for Norwegian). I ended up making my own wrapper for CultureInfo.GetCulture that, for certain cultures, clones the culture and applies some changes before returning the cloned culture. Then I (have to) remember to always call this instead of CultureInfo.GetCulture in all relevant server components. While I'd like a more robust way of globally overriding build-in cultures, I can live with this.


@tarekgh, thank you for the clarifications. I see now that I have misunderstood the purpose of TimeSeparator and of : in the time patterns.

As to your remark:

We have generic rule : represent the culture time separator placeholder. This rule works consistently and there is no confusion here. : is the only character that can be replaced by the actual culture time separator. Cannot assume using other character like . is going to behave the same.

To be clear, I never assumed that . was going to behave the same as : in terms of being a placeholder. My assumption was (past tense) that the time patterns would always use :, not ., so that setting the TimeSeparator would impact these.

Why is this not good enough for you?

It is good enough for me. I am able to format it exactly as I want by overriding the actual time patterns.

In hindsight, I also see that the TimeSeparator documentation states all the necessary information to arrive at this conclusion. However, it would have saved me hours of cross-environment debugging and this whole issue report if the documentation had been explicit about the simple fact that one can not expect : to be used in the time patterns, and therefore changes to TimeSeparator to have an effect on these. (I can imagine this is self-evident to you. It was not to me.)

For example, this paragraph could be extended (my insertion in italics):

If the custom pattern includes the format pattern ":", DateTime.ToString displays the value of TimeSeparator in place of the ":" in the format pattern. Note that standard format patterns, such as FullDateTimePattern, do not necessarily use ":". In other words, changing TimeSeparator may not have an effect when using these patterns.

@cmeeren
Copy link
Author

cmeeren commented Oct 15, 2022

If you're running on Windows, you're probably still hitting NLS, and using legacy data. See this page about the switch.

I read that page just now, but I don't understand why I should hit NLS. The page says near the top:

Windows 10 May 2019 Update and later versions include icu.dll as part of the OS, and .NET 5 and later versions use ICU by default.

I am using Windows 11 and .NET 6. However, when I try the code posted under the heading Determine if your app is using ICU, I get false, even if I use any combination of these elements to my project file:

<ItemGroup>
  <PackageReference Include="Microsoft.ICU.ICU4C.Runtime" Version="68.2.0.9" />
  <RuntimeHostConfigurationOption Include="System.Globalization.AppLocalIcu" Value="68.2" />
  <RuntimeHostConfigurationOption Include="System.Globalization.UseNls" Value="false" />
</ItemGroup>

I would like to ensure that I use the same settings across environments. Is there a way I can ensure I am using ICU?

@tarekgh
Copy link
Member

tarekgh commented Oct 15, 2022

@cmeeren thanks for your feedback, it is helpful.

  • For the documentation suggestion, feel free to open a PR and suggest any change and we'll be happy to accommodate it https://github.com/dotnet/dotnet-api-docs/blob/main/xml/System.Globalization/DateTimeFormatInfo.xml.
  • If you are running on Windows 11 client, you don't need to include anything. You should get ICU by default. could you please try run the following code and send the output of the version variable? This may help. Also, please ensure you remove any added related things from csproj.
            Type? interopGlobalization = Type.GetType("Interop+Globalization, System.Private.CoreLib");
            MethodInfo? methodInfo = interopGlobalization.GetMethod("GetICUVersion", BindingFlags.NonPublic | BindingFlags.Static);
            int version = methodInfo.Invoke(null, null)

I would like to ensure that I use the same settings across environments. Is there a way I can ensure I am using ICU?

Could you tell me what environments you are going to run in?

Note, Last 6.0 update (version 6.0.10) has a problem when using

<ItemGroup>
  <PackageReference Include="Microsoft.ICU.ICU4C.Runtime" Version="68.2.0.9" />
  <RuntimeHostConfigurationOption Include="System.Globalization.AppLocalIcu" Value="68.2" />
</ItemGroup>

We are working to fix that and hopefully will fix it soon.

@cmeeren
Copy link
Author

cmeeren commented Oct 16, 2022

I'll do that. 👍

could you please try run the following code and send the output of the version variable?

When run in an empty C# console app, it is 0. The project file is:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net6.0</TargetFramework>
    </PropertyGroup>

</Project>

I get the same result if I include the Microsoft.ICU.ICU4C.Runtime package and configure System.Globalization.AppLocalIcu.

This is also the case if I run it in an F# project. However, strangely enough, when running the code in F# interactive (in Rider 2022.2.3), I get 1140981766.

Could you tell me what environments you are going to run in?

Azure App Service, Windows server 2016 and newer, and local dev machines with Windows 11 21H2 and newer.

@tarekgh
Copy link
Member

tarekgh commented Oct 16, 2022

Could you add the following to the empty C# code project and send the result? It looks like you have something set in your environment.

            Console.WriteLine($".... UseNls:                 {typeof(object).Assembly.GetType("System.Globalization.GlobalizationMode")!.GetProperty("UseNls", BindingFlags.Static | BindingFlags.NonPublic)!.GetValue(null)} ....");
            Console.WriteLine($".... Invariant:              {typeof(object).Assembly.GetType("System.Globalization.GlobalizationMode")!.GetProperty("Invariant", BindingFlags.Static | BindingFlags.NonPublic)!.GetValue(null)} ....");

            Console.WriteLine($"OSDescription:        {RuntimeInformation.OSDescription}");
            Console.WriteLine($"FrameworkDescription: {RuntimeInformation.FrameworkDescription}");
            Console.WriteLine($"RuntimeIdentifier: {RuntimeInformation.RuntimeIdentifier}");

@cmeeren
Copy link
Author

cmeeren commented Oct 16, 2022

Thank you for helping me out! Here's the output:

.... UseNls:                 False ....
.... Invariant:              False ....
OSDescription:        Microsoft Windows 10.0.22000
FrameworkDescription: .NET 6.0.9
RuntimeIdentifier: win10-x64

(I have double-checked that I'm using Windows 11 Pro 21H2, build 22000.1098.)

@tarekgh
Copy link
Member

tarekgh commented Oct 16, 2022

This means the ICU is used in the app. What makes you think the ICU is not loaded? Do you see different behavior?

@cmeeren
Copy link
Author

cmeeren commented Oct 17, 2022

Okay, this is very weird.

Consider this code that you previously sent me:

Type? interopGlobalization = Type.GetType("Interop+Globalization, System.Private.CoreLib");
MethodInfo? methodInfo = interopGlobalization.GetMethod("GetICUVersion", BindingFlags.NonPublic | BindingFlags.Static);
Console.WriteLine(methodInfo.Invoke(null, null));

When executed on its own in a blank C# project, it prints 0.

However, if I add either of these lines before the code above, it returns 1140981766:

typeof(object).Assembly.GetType("System.Globalization.GlobalizationMode")!.GetProperty("UseNls", BindingFlags.Static | BindingFlags.NonPublic)!.GetValue(null);
SortVersion sortVersion = CultureInfo.InvariantCulture.CompareInfo.Version;

(The last line above is from Determine if your app is using ICU.)

Repro solution (though I'm not sure it helps, if it's related to the environment):

Repro.zip

@tarekgh
Copy link
Member

tarekgh commented Oct 17, 2022

You are getting 0 because you have been calling the ICU version early in your app before the globalization code get initialized. Just add a line like the following in the top of your Main method and you should see the correct ICU version at that time.

CultureInfo ci = CultureInfo.GetCultureInfo("ja-JP");

@cmeeren
Copy link
Author

cmeeren commented Oct 18, 2022

Thanks. It seems it's the opposite, then. My local Win 11 machine is using ICU (1140981766), and Azure App Service is not (0).

@tarekgh
Copy link
Member

tarekgh commented Oct 18, 2022

and Azure App Service is not (0).

Most likely that is because the service is running on Windows Server 2019. We have changed that in .NET 7.0 to allow using ICU automatically on Windows Server 2019 #72656.

@cmeeren
Copy link
Author

cmeeren commented Oct 18, 2022

Thanks for the note. For consistency though, and to avoid future issues where the behavior is different across environments, I will likely stick to app-local ICU going forward.

@tarekgh
Copy link
Member

tarekgh commented Oct 18, 2022

I will likely stick to app-local ICU going forward.

In .NET 7.0, we have fixed loading ICU on Windows Server 2019. Microsoft no longer supports the older versions of Windows. Theoretically, if you use .NET 7.0, you'll not need app local. Anyway, it is your choice to evaluate your situation and do what you think is proper. Let me know if you have any questions we can help with.

@cmeeren
Copy link
Author

cmeeren commented Oct 18, 2022

Great, it seems like I can then safely remove app-local ICU after updating to .NET 7. I have no further questions. Thank you so much for all the help!

@cmeeren
Copy link
Author

cmeeren commented Oct 18, 2022

Oh, actually, one more thing: If I use .NET 7 preview on Azure App Service now, do you know if I will get the behavior you describe (ICU without having to use app-local)?

@tarekgh
Copy link
Member

tarekgh commented Oct 18, 2022

Yes, using the latest preview you should get the behavior you desire. give it a try and let me know if you see anything there.

@cmeeren
Copy link
Author

cmeeren commented Oct 19, 2022

Unfortunately, this seemed to have no effect. I published a self-contained app using .NET 7 RC2 without app-local ICU to Azure App Service, and it's not using ICU.

@tarekgh
Copy link
Member

tarekgh commented Oct 19, 2022

Unfortunately, this seemed to have no effect. I published a self-contained app using .NET 7 RC2 without app-local ICU to Azure App Service, and it's not using ICU.

How did you know that it is not using ICU? Could you send some more info? What OS was used on Azure in your run?

#72656

@cmeeren
Copy link
Author

cmeeren commented Oct 19, 2022

GetICUVersion returns 0 (I'm invoking it a short while after app startup, not immediately). If I use app-local ICU, it returns another version.

I'm using Azure App Service on Windows in the Norway East region. Not sure what it's running, or how to find out. Do you know?

@tarekgh
Copy link
Member

tarekgh commented Oct 19, 2022

Log the following informaition which can give you the needed info:

            Console.WriteLine($"OSDescription:        {RuntimeInformation.OSDescription}");
            Console.WriteLine($"FrameworkDescription: {RuntimeInformation.FrameworkDescription}");
            Console.WriteLine($"RuntimeIdentifier: {RuntimeInformation.RuntimeIdentifier}");

@cmeeren
Copy link
Author

cmeeren commented Oct 19, 2022

OSDescription: Microsoft Windows 10.0.14393
FrameworkDescription: .NET 6.0.8
RuntimeIdentifier: win10-x64

@tarekgh
Copy link
Member

tarekgh commented Oct 19, 2022

FrameworkDescription: .NET 6.0.8 so you are not using .NET 7.0. Try to ensure you are building and publishing with .NET 7.0.

@cmeeren
Copy link
Author

cmeeren commented Oct 20, 2022

Sorry, that was after I reverted back to .NET 6 because it didn't work.

I am 99% sure I was actually running .NET 7, but I will report back when I try again.

@cmeeren
Copy link
Author

cmeeren commented Oct 20, 2022

Actually though, the issue seems to be that Azure App Service uses Windows Server 2016, as far as I can tell from the version number. Isn't that right?

@tarekgh
Copy link
Member

tarekgh commented Oct 20, 2022

You are right. The Windows version is Windows Server 2016. Unfortunetly, you'll need to use ICU app-local for that because this server doesn't carry ICU at all.

@cmeeren
Copy link
Author

cmeeren commented Oct 20, 2022

Thanks, glad we clarified that. 🙂 I will continue using app-local ICU. (To avoid #77045 I am deploying self-contained version produced using a locked version with global.json.)

@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants