Add -Name, -NoUserOverrides and -ListAvailable parameters to Get-Culture cmdlet #7702

iSazonov · 2018-09-04T15:00:45Z

PR Summary

Address #3832

Add new parameters in Get-Culture cmdlet:

-Name - to allow retrieving a specific culture
-NoUserOverrides - ignore user changes for current culture
-ListAvalable - to allow retrieving all cultures supported on the platform

Documentation

Please see https://github.com/dotnet/corefx/issues/6374#issuecomment-418827420 and following comments.
In short .Net Core accept all culture names in BCP-47 format and then delegate to OS so result depends on OS behavior and differ on different OS-s.

PR Checklist

iSazonov · 2018-09-04T15:08:55Z

/cc @mklement0

mklement0 · 2018-09-04T15:38:11Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetCultureCommand.cs

+        private const string ListAvailableParameterSet = "ListAvailable";
+
+        /// <summary>
+        /// Gets and sets the specified culture.


As an aside: I don't think it's worth reflecting that the property representing the parameter is read/write (that can easily be gleaned from the code) - just stating the purpose of the parameter should be sufficient.

The comment should start with "Gets and sets" to fix CodeFactor issue.

I see, but in this context such comments are more confusing than helpful. I know nothing about CodeFactor: does this particular rule a default rule, or one that was specifically turned on for this repo?

Native language is very flexible - I reworded the comment.

CodeFactor is a service. It uses StypeCop static analyzer too. Yes, we can to turn in/out rules. Currently we have some PRs to turn StypeCop and editors settings. Plan is to complete the work after 6.1 GA.

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetCultureCommand.cs

SteveL-MSFT · 2018-09-04T17:11:03Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetCultureCommand.cs

        /// <summary>
        /// Output the current Culture info object.
        /// </summary>
-        protected override void BeginProcessing()
+        protected override void EndProcessing()


Is there a reason to do this in EndProcessing vs ProcessRecord? Typically, EndProcessing is used for cleanup after the pipeline has completed.

Initially we do not support pipelines and using EndProcessing() was ok. Currently the cmdlet was enhanced to support pipelines and the code was moved to ProcessRecord().

iSazonov · 2018-09-05T05:21:43Z

I discovered that

Get-Culture "abc"

return CultureInfo object - there is no exception internally. Seems it is "by design" and based on OS behavior. See https://github.com/dotnet/corefx/issues/1669. Perhaps we should add this in documentation.

mklement0 · 2018-09-05T05:40:43Z

@iSazonov: That is unfortunate; ~~the problem has since been fixed on Windows, but macOS and Linux are still affected.~~ - see below.

iSazonov · 2018-09-05T06:08:19Z

I see. I'll skip the test on Linux and MacOs. Should I add a comment that it is a bug in .Net Core? Is there a tracking issue?

mklement0 · 2018-09-05T06:21:39Z

I would personally consider it a bug, but I've only read the CoreFx issue that you linked to; from there I came across https://github.com/dotnet/corefx/issues/6374#issuecomment-245706306, which promises a fix on Linux, but hat seemingly never happened.

May be worth opening a new issue.

iSazonov · 2018-09-05T06:41:29Z

@mklement0 Thanks! I added a comment there.

dantraMSFT · 2018-09-05T20:37:21Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Culture.Tests.ps1

+        $ciArray[0] | Should -BeOfType [CultureInfo]
+    }
+
+    It "Should write an error on unsupported culture name" -Skip:(!$isWindows) {


I suggest adding the issue link as a comment for -Skip

https://github.com/dotnet/corefx/issues/6374

I updated the test - now it throw on Unix-es too. Link to useful comment added too.

dantraMSFT · 2018-09-05T20:41:41Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetCultureCommand.cs

+        /// Gets or sets culture names for which CultureInfo values are returned.
+        /// </summary>
+        [Parameter(ParameterSetName = NameParameterSet, Position = 0, ValueFromPipeline = true)]
+        [ValidateNotNull]


Shouldn't this be ValidateNotNullOrEmpty?

For empty string name InvariantCulture is returned.

@iSazonov - I think that behavior's too subtle.

Additionally, the ProcessRecord doesn't match the class comment (Return's the thread's current culture). In short, reviewing the code appears to not support getting the current culture.

NOTE: Line 40 that compares Name to null 'looks' like dead code since name must be non-null for the Name parameter set. Having an explicit or fall through 'default' case statement will make this clearer.

I suggest the following:

1: Retain the original behavior explicitly
Set the DefaultParameterSetName to 'default' and return the thread's current culture.

2: Name parameter set
Either clearly document that an empty string returns InvariantCulture or don't allow empty strings and require explicit names.

3: ListAvailable
As implemented.

I too think that "" is obscure and suggest requiring explicit use of Invariant-Culture, i.e.:

Get-Culture Invariant-Culture

1 - Fixed.
2- Let's consider the scenario - script asks user to input a culture, user press Enter to choose "Default". Without user typing the script get empty string as "default". I guess CoreFX consider InvariantCulture as default if culture is unknown or not selected. We use InvariantCulture widely in PowerShell code too. So allowing empty string in Get-Culture looks good.

Re 2: Curiously, the empty string is not just a placeholder that signals the intent to retrieve the invariant culture when you call .GetCultureInfo(), it is the actual value of the .Name property of the culture-info object returned - so I guess it makes sense to surface that.(Unlike what I thought earlier, "Invariant-Culture" doesn't actually work; on Unix it implicitly creates an ad-hoc culture, on Windows it breaks.)

Perhaps add a comment about this in the code

SteveL-MSFT · 2018-09-05T22:17:07Z

@PowerShell/powershell-committee reviewed this and continue to be consistent in our thinking that for literals, a non-terminating error should be returned, and for filtering/wildcards, it should return nothing if not found. Should also consider adding a Test-Culture cmdlet.

mklement0 · 2018-09-06T01:21:39Z

Sorry - my earlier statement was incorrect:

The - questionable - logic of the CultureInfo.GetCultureInfo() method is the same across all platforms in the abstract:

A culture name that is formally correct returns a culture-info object, even if there is no such predefined culture - I'm unclear on when and why that is useful.
A formally incorrect culture name triggers a System.Globalization.CultureNotFoundException, which makes sense.

What constitutes a formally correct culture name is platform-specific, however:

Windows: a well-formed BCP-47 tag, such as fr, ru-RU, or zz-AA-oooooooo
Unix (macOS and Linux): a name composed of up to 11 characters, whatever they are, and seemingly even more if there's a . present (I'm unclear on the exact rules); e.g., 01;2.45"78&$ (sic); according to the linked CoreFx issue, the "ICU library" is used behind the scenes on these platforms.

mklement0 · 2018-09-06T01:42:50Z

I guess we can work around this by validating names against CultureInfo.GetCultures(CultureTypes.AllCultures) while we're waiting for a CoreFX fix - which may or may not come.

@SteveL-MSFT: Currently, the PR has no wildcard support for -Name - should we add it? Might be handy, but adds implementation complexity.

iSazonov · 2018-09-06T04:01:11Z

@SteveL-MSFT

Should also consider adding a Test-Culture cmdlet.

We have #7562. I suggest to explicitly track all approved Test-* cmdlets there.

iSazonov · 2018-09-06T04:07:59Z

I guess we can work around this

@mklement0 I think it is ok to follow .Net Core behavior "as is" - if they really think it's right, then we can hardly do better.

mklement0 · 2018-09-06T12:40:40Z

@iSazonov: Since we're not just providing access to the underlying CoreFx functionality, but packaging it in a PowerShell-appropriate way, I don't think we're obligated to surface the functionality as-is.

I have yet to hear a good reason for the CoreFx behavior (there may be one, but I haven't heard it), and users who really want to implicitly create cultures such as zz-AA and 01;2.45"78&$ can use the CoreFx types directly.

iSazonov · 2018-09-06T15:20:48Z

@mklement0 With -ListAvailable we get good UX. We could add a filter for -Name (based on a completer) but it can be expensive and it seems we do not fully understand the behavior of .Net Core in order to overlap it. I think we shouldn't delay the PR, we can postpone this and fix later if needed.

dantraMSFT · 2018-09-06T15:51:52Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetCultureCommand.cs

+        /// Gets or sets culture names for which CultureInfo values are returned.
+        /// </summary>
+        [Parameter(ParameterSetName = NameParameterSet, Position = 0, ValueFromPipeline = true)]
+        [ValidateNotNull]


@iSazonov - I think that behavior's too subtle.

Additionally, the ProcessRecord doesn't match the class comment (Return's the thread's current culture). In short, reviewing the code appears to not support getting the current culture.

NOTE: Line 40 that compares Name to null 'looks' like dead code since name must be non-null for the Name parameter set. Having an explicit or fall through 'default' case statement will make this clearer.

I suggest the following:

1: Retain the original behavior explicitly
Set the DefaultParameterSetName to 'default' and return the thread's current culture.

2: Name parameter set
Either clearly document that an empty string returns InvariantCulture or don't allow empty strings and require explicit names.

3: ListAvailable
As implemented.

mklement0 · 2018-09-06T15:53:17Z

@iSazonov: Postponing wildcard support sounds good to me, but I do think we should get clarity on whether we truly want to support calls such as Get-Culture '01;2.45"78&$', because if we do so now, we have a breaking change on our hands if we want to change it later.

I see only down-sides to surfacing the baffling CoreFx functionality as-is.

Conversely, if we restrict allowed names to the ones reported by -ListAvailable now, it is less problematic to loosen that restriction later, if it turns out that that is really necessary.

iSazonov · 2018-09-07T11:36:51Z

whether we truly want to support calls such as Get-Culture '01;2.45"78&$', because if we do so now, we have a breaking change on our hands if we want to change it later.

I don't know whether we truly want. The breaking change will be in grace arae - I'd expect users will use right culture names. On the other hand, it is not clear what the filter should be. from the CoreFX discussion we know that there may be some special logic. There can be a situation similar we see with encodings - we get a short list but really supported encodings there is more.
I suggest document the feature and wait feedback.

iSazonov · 2018-10-12T09:14:45Z

@SteveL-MSFT @mklement0 The PR is ready for final review.

SteveL-MSFT · 2018-10-12T17:39:59Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetCultureCommand.cs

+        /// Gets or sets culture names for which CultureInfo values are returned.
+        /// </summary>
+        [Parameter(ParameterSetName = NameParameterSet, Position = 0, ValueFromPipeline = true)]
+        [ValidateNotNull]


Perhaps add a comment about this in the code

SteveL-MSFT · 2018-10-12T17:41:42Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetCultureCommand.cs

+                            }
+                            else
+                            {
+                                ci = CultureInfo.GetCultureInfo(cultureName);


Agree, based on @PowerShell/powershell-committee discussion we should only return cultures from the AllCultures list. Creation is out of scope of this PR.

SteveL-MSFT · 2018-10-12T17:44:45Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Culture.Tests.ps1

+
+    It "Should have `$PSCulture variable be equivalent to (Get-Culture).Name" {
+
+        (Get-Culture).Name | Should -Be $PsCulture


Based on the test description, it would seem that this should be:

$PSCulture | Should -BeExactly (Get-Culture).Name

I think the order is right because the file is for Get-Culture tests and we should correct the description.
But I'd expect that we check $Host.CurrentCulture instead of $PsCulture.
On other side, we haven't tests for special variables at all.

What do you think?

I'm fine changing the test description to match the test and adding separate test for $PSCulture

SteveL-MSFT · 2018-10-12T17:46:12Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Culture.Tests.ps1

+
+    It "Should return specified cultures with '-Name' parameter" {
+
+        $ciArray = Get-Culture "", "ru-RU"


Seems like we should have a specific test case to handle invariant culture making it clear what is being validated

Added the check and comment.

SteveL-MSFT · 2018-10-12T17:48:42Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetCultureCommand.cs

+                        {
+                            if (NoUserOverrides && string.Equals(cultureName, Host.CurrentCulture.Name, StringComparison.CurrentCultureIgnoreCase))
+                            {
+                                ci = new CultureInfo(cultureName, false);


Please use named params here

mklement0 · 2018-10-12T18:14:45Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetCultureCommand.cs

+                case CurrentCultureParameterSet:
+                    if (NoUserOverrides)
+                    {
+                        ci = CultureInfo.GetCultureInfo(Host.CurrentCulture.LCID);


Please use .Name instead of .LCID, and we should probably do the same for all tests.

"LCIDs are being deprecated, and implementers are strongly encouraged to use locale names instead. LCIDs can be used for backward compatibility, but as noted in section 2.2.1, there is no guarantee of LCID uniqueness when used with valid locale names not otherwise associated with an LCID. " - from "[MS-LCID]: Windows Language Code Identifier (LCID) Reference" - https://winprotocoldoc.blob.core.windows.net/productionwindowsarchives/MS-LCID/[MS-LCID].pdf

I think the change is not related to and not critical for the PR. We can postpone the change until CoreFX obsolete the API.

[cultureinfo]::getcultures('allcultures') | group-object lcid | ? count -gt 1 will tell you that hundreds of predefined cultures share the same LCID, so this fix is an absolute must.

mklement0 · 2018-10-12T18:53:12Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetCultureCommand.cs

+                    {
+                        foreach (var cultureName in Name)
+                        {
+                            if (NoUserOverrides && string.Equals(cultureName, Host.CurrentCulture.Name, StringComparison.CurrentCultureIgnoreCase))


This should be !NoUserOverrides and the 2nd CultureInfo constructor argument below should be true (or omitted), but you could just use Host.CurrentCulture, which also reflects the overrides.

iSazonov · 2018-10-16T03:58:33Z

@SteveL-MSFT Please point me comments if I missed something.

SteveL-MSFT

LGTM

iSazonov · 2018-10-18T04:47:29Z

@mklement0 Please update your review.

iSazonov · 2018-10-18T04:47:58Z

Reopen to restart all CIs.

iSazonov · 2018-10-18T12:35:50Z

@mklement0 Many thanks for great help!

mklement0 · 2018-10-18T12:44:55Z

My pleasure, @iSazonov, thanks for implementing it all.

…ure cmdlet (PowerShell#7702) Add new parameters in Get-Culture cmdlet: -Name - to allow retrieving a specific culture -NoUserOverrides - ignore user changes for current culture -ListAvalable - to allow retrieving all cultures supported on the platform

iSazonov added Review - Committee The PR/Issue needs a review from the PowerShell Committee Documentation Needed in this repo Documentation is needed in this repo labels Sep 4, 2018

iSazonov requested review from dantraMSFT, JamesWTruher and PaulHigin as code owners September 4, 2018 15:00

iSazonov requested a review from SteveL-MSFT September 4, 2018 15:01

iSazonov force-pushed the get-culture branch from d7afc9a to 64e1f70 Compare September 4, 2018 15:07

mklement0 reviewed Sep 4, 2018

View reviewed changes

mklement0 suggested changes Sep 4, 2018

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/commands/utility/GetCultureCommand.cs Outdated Show resolved Hide resolved

SteveL-MSFT requested changes Sep 4, 2018

View reviewed changes

dantraMSFT suggested changes Sep 5, 2018

View reviewed changes

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Sep 5, 2018

SteveL-MSFT mentioned this pull request Sep 6, 2018

There should be more Test-Command and Test-Module cmdlets #7562

Closed

1 task

dantraMSFT suggested changes Sep 6, 2018

View reviewed changes

Address PowerShell Committee conclusion

173849d

iSazonov force-pushed the get-culture branch from 4abc5df to 173849d Compare October 12, 2018 08:59

SteveL-MSFT requested changes Oct 12, 2018

View reviewed changes

mklement0 suggested changes Oct 12, 2018

View reviewed changes

iSazonov added 3 commits October 15, 2018 12:20

Address feedbacks

14b8e7b

Skip test on Unix

7bcee51

Replace LCID with name

d00a899

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Add -Name, -ListAvailable, and -NoUserOverrides parameters to Get-Culture cmdlet' MicrosoftDocs/PowerShell-Docs#3025

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

Change tests description and add new test

c12d1a1

SteveL-MSFT approved these changes Oct 18, 2018

View reviewed changes

iSazonov closed this Oct 18, 2018

iSazonov reopened this Oct 18, 2018

mklement0 approved these changes Oct 18, 2018

View reviewed changes

Fix test

a54d534

iSazonov mentioned this pull request Oct 18, 2018

New parameters in Get-Culture cmdlet MicrosoftDocs/PowerShell-Docs#3138

Closed

8 tasks

iSazonov changed the title ~~Add -Name and -ListAvailable parameters to Get-Culture cmdlet~~ Add -Name, -NoUserOverrides and -ListAvailable parameters to Get-Culture cmdlet Oct 18, 2018

iSazonov merged commit f1e2136 into PowerShell:master Oct 18, 2018

iSazonov deleted the get-culture branch October 18, 2018 12:34

mklement0 mentioned this pull request Jan 31, 2020

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

Closed


		It "Should have `$PSCulture variable be equivalent to (Get-Culture).Name" {

		(Get-Culture).Name \| Should -Be $PsCulture


		It "Should return specified cultures with '-Name' parameter" {

		$ciArray = Get-Culture "", "ru-RU"

Add -Name, -NoUserOverrides and -ListAvailable parameters to Get-Culture cmdlet #7702

Add -Name, -NoUserOverrides and -ListAvailable parameters to Get-Culture cmdlet #7702

Conversation

iSazonov commented Sep 4, 2018 • edited

PR Summary

Documentation

PR Checklist

iSazonov commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Sep 5, 2018

mklement0 commented Sep 5, 2018 • edited

iSazonov commented Sep 5, 2018

mklement0 commented Sep 5, 2018

iSazonov commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveL-MSFT commented Sep 5, 2018

mklement0 commented Sep 6, 2018 • edited

mklement0 commented Sep 6, 2018

iSazonov commented Sep 6, 2018

iSazonov commented Sep 6, 2018

mklement0 commented Sep 6, 2018

iSazonov commented Sep 6, 2018

Choose a reason for hiding this comment

mklement0 commented Sep 6, 2018 • edited

iSazonov commented Sep 7, 2018 • edited

iSazonov commented Oct 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Oct 16, 2018

SteveL-MSFT left a comment

Choose a reason for hiding this comment

iSazonov commented Oct 18, 2018

iSazonov commented Oct 18, 2018

iSazonov commented Oct 18, 2018

mklement0 commented Oct 18, 2018

iSazonov commented Sep 4, 2018 •

edited

mklement0 commented Sep 5, 2018 •

edited

mklement0 commented Sep 6, 2018 •

edited

mklement0 commented Sep 6, 2018 •

edited

iSazonov commented Sep 7, 2018 •

edited