Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,23 +1,124 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Management.Automation;

namespace Microsoft.PowerShell.Commands
{
/// <summary>
/// Returns the thread's current culture.
/// Returns:
/// - the thread's current culture
/// - culture by name
/// - list of all supported cultures.
/// </summary>
[Cmdlet(VerbsCommon.Get, "Culture", HelpUri = "https://go.microsoft.com/fwlink/?LinkID=113312")]
[Cmdlet(VerbsCommon.Get, "Culture", DefaultParameterSetName = CurrentCultureParameterSet, HelpUri = "https://go.microsoft.com/fwlink/?LinkID=113312")]
[OutputType(typeof(System.Globalization.CultureInfo))]
public sealed class GetCultureCommand : PSCmdlet
{
private const string CurrentCultureParameterSet = "CurrentCulture";
private const string NameParameterSet = "Name";
private const string ListAvailableParameterSet = "ListAvailable";

/// <summary>
/// Gets or sets culture names for which CultureInfo values are returned.
/// Empty string matches Invariant culture.
/// </summary>
[Parameter(ParameterSetName = NameParameterSet, Position = 0, ValueFromPipeline = true)]
[ValidateSet(typeof(ValidateCultureNamesGenerator))]
[ValidateNotNull]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be ValidateNotNullOrEmpty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For empty string name InvariantCulture is returned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Get-Culture Invariant-Culture

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a comment about this in the code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

public string[] Name { get; set; }

/// <summary>
/// Gets or sets a switch to return current culture with user overrides (by default).
/// With the switch on, we return current culture without user overrides.
/// </summary>
[Parameter(ParameterSetName = CurrentCultureParameterSet)]
[Parameter(ParameterSetName = NameParameterSet)]
public SwitchParameter NoUserOverrides { get; set; }

/// <summary>
/// Gets or sets a switch to list all available cultures.
/// </summary>
[Parameter(ParameterSetName = ListAvailableParameterSet)]
public SwitchParameter ListAvailable { get; set; }

/// <summary>
/// Output the current Culture info object.
/// Output:
/// - the thread's current culture
/// - culture by name
/// - list of all supported cultures.
/// </summary>
protected override void BeginProcessing()
protected override void ProcessRecord()
{
CultureInfo ci;

switch (ParameterSetName)
{
case CurrentCultureParameterSet:
if (NoUserOverrides)
{
ci = CultureInfo.GetCultureInfo(Host.CurrentCulture.Name);
}
else
{
ci = Host.CurrentCulture;
}

WriteObject(ci);

break;
case NameParameterSet:
try
{
foreach (var cultureName in Name)
{
if (!NoUserOverrides && string.Equals(cultureName, Host.CurrentCulture.Name, StringComparison.CurrentCultureIgnoreCase))
{
ci = Host.CurrentCulture;
}
else
{
ci = CultureInfo.GetCultureInfo(cultureName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given my discussions in the CoreFx repo, I still feel strongly that we should validate each cultureName against the list of predefined cultures reported by CultureInfo.GetCultures(CultureTypes.AllCultures).

There is no good reason to follow the - IMO misguided - CoreFx behavior. No one should expect a cmdlet named Get-Culture to create cultures on demand, likely unintentionally. It is much more useful to be notified of having accidentally typed the name of a non-predefined culture by way of an error.

We can allow ad-hoc creation on an opt-in basis with another switch, say -CreateOnDemand, but I'm not sure that's even necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

WriteObject(ci);
}
}
catch (CultureNotFoundException exc)
{
WriteError(new ErrorRecord(exc, "ItemNotFoundException", ErrorCategory.ObjectNotFound, Name));
}

break;
case ListAvailableParameterSet:
foreach (var cultureInfo in CultureInfo.GetCultures(CultureTypes.AllCultures))
{
WriteObject(cultureInfo);
}

break;
}
}
}

/// <summary>
/// Get list of valid culture names for ValidateSet attribute.
/// </summary>
public class ValidateCultureNamesGenerator : IValidateSetValuesGenerator
{
string[] IValidateSetValuesGenerator.GetValidValues()
{
WriteObject(Host.CurrentCulture);
var cultures = CultureInfo.GetCultures(CultureTypes.AllCultures);
var result = new List<string>(cultures.Length);
foreach (var cultureInfo in cultures)
{
result.Add(cultureInfo.Name);
}

return result.ToArray();
}
}
}
Original file line number Diff line number Diff line change
@@ -1,25 +1,72 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.
Describe "Get-Culture DRT Unit Tests" -Tags "CI" {
It "Should works proper with get-culture" {
$results = get-Culture
$results -is "System.Globalization.CultureInfo" | Should -BeTrue
$results[0].Name | Should -Be $PSCulture
}
}

Describe "Get-Culture" -Tags "CI" {

It "Should return a type of CultureInfo for Get-Culture cmdlet" {

Get-Culture | Should -BeOfType CultureInfo
$culture = Get-Culture
$culture | Should -BeOfType [CultureInfo]
($culture).EnglishName | Should -BeExactly $host.CurrentCulture.EnglishName

Get-Culture -NoUserOverrides | Should -BeOfType [CultureInfo]
}

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

(Get-Culture).Name | Should -BeExactly $PsCulture
}

It "Should have $ culture variable be equivalent to (Get-Culture).Name" {
It "Should return the specified culture with '-Name' parameter" {

(Get-Culture).Name | Should -Be $PsCulture
$ci = Get-Culture -Name ru-RU
$ci | Should -BeOfType [CultureInfo]
$ci.Name | Should -BeExactly "ru-RU"

$ci = Get-Culture -Name ru-RU -NoUserOverrides
$ci | Should -BeOfType [CultureInfo]
$ci.Name | Should -BeExactly "ru-RU"
}

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

$ciArray = Get-Culture "", "ru-RU"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the check and comment.

$ciArray | Should -HaveCount 2
$ciArray[0] | Should -BeOfType [CultureInfo]
$ciArray[0].EnglishName | Should -BeExactly "Invariant Language (Invariant Country)"

$ciArray[1] | Should -BeOfType [CultureInfo]
$ciArray[1].Name | Should -BeExactly "ru-RU"
$ciArray[1].EnglishName | Should -BeExactly "Russian (Russia)"
}

It "Should accept values from a pipeline for '-Name' parameter" {

$ciArray = "", "ru-RU" | Get-Culture
$ciArray | Should -HaveCount 2
$ciArray[0] | Should -BeOfType [CultureInfo]
$ciArray[0].EnglishName | Should -BeExactly "Invariant Language (Invariant Country)"
$ciArray[1] | Should -BeOfType [CultureInfo]
$ciArray[1].Name | Should -BeExactly "ru-RU"
$ciArray[1].EnglishName | Should -BeExactly "Russian (Russia)"
}

It "Should return the culture array with '-ListAvailable' parameter" {

$ciArray = Get-Culture -ListAvailable
$ciArray.Count | Should -BeGreaterThan 0
$ciArray[0] | Should -BeOfType [CultureInfo]
}

It "Should write an error on unsupported culture name" {

{ Get-Culture -Name "abcdefghijkl" -ErrorAction Stop } | Should -PassThru -Throw -ErrorId "ParameterArgumentValidationError,Microsoft.PowerShell.Commands.GetCultureCommand"
}
}

Describe "`$PSCulture" -Tags "CI" {

It "Check `$PSCulture value" {
$PSCulture | Should -BeExactly $([System.Globalization.CultureInfo]::CurrentCulture.Name)
}
}