GeezerVison

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.

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().

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.

Shouldn't this be ValidateNotNullOrEmpty?

For empty string name InvariantCulture is returned.

Ilya (@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

Added.

Ilya (@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.

Perhaps add a comment about this in the code

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

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

Fixed.

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.

Please use named params here

Fixed.

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.

Fixed.

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.

Fixed.