dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.16k stars 4.72k forks source link

API proposal: Extend CultureInfo.GetCultureInfo and new CultureInfo to optionally accept predefined cultures only #27352

Closed mklement0 closed 4 years ago

mklement0 commented 6 years ago

Follow-up from dotnet/runtime#16457

Rationale and Use Cases

Surprisingly, since Windows 10 and on Unix-like platforms, CultureInfo.GetCultureInfo(<name>) and also new CultureInfo(<name>) implicitly create (custom) cultures, if <name> does not refer to any of the cultures reported by CultureInfo.GetCultures(CultureTypes.AllCultures) - as long as <name> is formally a valid culture name (which varies by platform).

This makes it cumbersome to ensure that a given culture name refers to a predefined culture - manual validation of the name of the CultureInfo instance returned against the list of predefined cultures reported by CultureInfo.GetCultures(CultureTypes.AllCultures) is necessary.

Arguably, throwing an exception if a non-predefined culture name is used should be the default (whereas creation of a custom culture should be a deliberate act), but changing this is no longer an option.

An example use case is a helper function in PowerShell for convenient testing of culture-sensitive functionality, Use-Culture, to which you pass the name of a predefined culture; e.g., Use-Culture de-DE { 1.2 } outputs 1,2, reflecting the culture-specific decimal mark, ,. The intent is to test with predefined cultures, so a typo such as Use-Culture de_DE { 1.2 } should result in an error, as opposed to - unintended - ad-hoc creation of a culture.
The proposed API extension would would allow the function to test the validity of a given culture name directly, without having to search through the array of predefined cultures returned by CultureInfo.GetCultures(CultureTypes.AllCultures).

A pending enhancement to the Get-Culture cmdlet would benefit similarly.

Proposed API

Introduce a Boolean predefinedOnly parameter which ensures that only the names of predefined cultures are accepted and throws a System.Globalization.CultureNotFoundException exception otherwise:

public static System.Globalization.CultureInfo GetCultureInfo (string name, bool predefinedOnly);

// Note: Since a CultureInfo (string, bool) constructor already exists, the parameter can only
//           be *added* to it.
public CultureInfo (string name, bool useUserOverride, bool predefinedOnly);

Note that both signatures are needed, because only the constructor form enables retrieval of culture information that reflects user overrides (user-specific tweaks to a predefined culture via Control Panel).

@tarekgh suggests holding off on implementing the following signature until the need arises, if ever:

public static System.Globalization.CultureInfo GetCultureInfo (string name, string altName, bool predefinedOnly);
tarekgh commented 6 years ago

Thanks, @mklement0

Do we really need

public static System.Globalization.CultureInfo GetCultureInfo (string name, string altName, bool predefinedOnly);

Also, I don't think we can have

public CultureInfo (string name, bool predefinedOnly);

because we already have the constructor that takes a string and boolean parameters. this need to change.

Would it be better to have TryGetCultureInfo family which can return bool and not throwing?


static bool TryeGetCultureInfo(string name, out CultureInfo culture);
static bool TryeGetCultureInfo(string name, bool userOverride, out CultureInfo culture);
static bool TryeGetCultureInfo(string name, bool userOverride, bool predefinedOnly, out CultureInfo culture);
mklement0 commented 6 years ago

Good point re public CultureInfo (string name, bool predefinedOnly), thanks - I've removed it from the OP.


Re whether we need GetCultureInfo (string name, string altName, bool predefinedOnly):

I think so, yes, because it's likely that you want to ensure that altName too refers to an existing culture.


While also having TryGetCultureInfo methods wouldn't hurt, I wouldn't only provide that, because if the user's intent to is retrieve a preexisting culture that they assume indeed exists, it is awkward to frame that in terms of a try - Try*() methods are for situations where you anticipate failure, which is not the case here.

tarekgh commented 6 years ago

I think so, yes, because it's likely that you want to ensure that altName too refers to an existing culture.

I am asking if you are going to use it? we can expose this later as needed. I want to ensure we expose only what is needed in your scenario.

For TryGetCultureInfo, I'll not add here for now if we are going to expose a new constructors or GetCultureInfo override

mklement0 commented 6 years ago

I am asking if you are going to use it?

No, in the context of PowerShell I'm not aware of a need for a GetCultureInfo (string name, string altName) overload that enforces use of preexisting culture names only.

Out of curiosity, however: why not make that change too, for consistency? Almost seems easier than leaving users to wonder about this omission.

tarekgh commented 6 years ago

Out of curiosity, however: why not make that change too, for consistency? Almost seems easier than leaving users to wonder about this omission.

Your scenario is really not the main scenario. I am not expecting many others will use this. this is why I don't want to expose something nobody else will use it. if we get a request later to have it, we can add it at any time.

tarekgh commented 6 years ago

@mklement0 could you please update the top description with the final proposal as we discussed it here? please follow the proposal format (look at the issue as an example https://github.com/dotnet/corefx/issues/28944). thanks.

mklement0 commented 6 years ago

@tarekgh, please see if my update to the OP is what you had in mind.

tarekgh commented 6 years ago

please see if my update to the OP is what you had in mind.

LGTM. thanks.

tarekgh commented 6 years ago

cc @krwq

terrajobst commented 5 years ago

Video

After some debate we settled on this:

namespace System.Globalization
{
    public partial class CultureInfo
    {
        public static CultureInfo GetCultureInfo(string name, bool prefinedOnly);
    }
}

This API would throw for invalid names as well as names that weren't system provided when predefinedOnly is true.

terrajobst commented 5 years ago

Correct. I've fixed the comment.

tarekgh commented 4 years ago

Done by the PR https://github.com/dotnet/runtime/pull/1261