Skip to content

Fix PSSA for Turkish culture and tests when culture is not en-US #1099

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.

Sign up for GitHub

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

Merged
merged 20 commits into from
Mar 8, 2019
Merged

Fix PSSA for Turkish culture and tests when culture is not en-US #1099

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
010a862
Fix Parsing of settings that occurs when using turkish culture
bergmeister Nov 6, 2018
57ab21c
use german culture before running tests to exhibit failures
bergmeister Nov 8, 2018
dcfe1cf
set ui culture as well
bergmeister Nov 8, 2018
5fea008
fix communityanalyzer for turkish culture
bergmeister Nov 8, 2018
a8df05d
fix ProvideCommentHelp for turkish culture (i problem again)
bergmeister Nov 8, 2018
c33c38b
update comments
bergmeister Nov 8, 2018
479ed3a
fix for import-localizedDatabug in pscore (opened issue 8219 in ps co…
bergmeister Nov 9, 2018
227b64f
change to UI culture
bergmeister Nov 9, 2018
cee22f9
use invariant culture comparison instead of tolowerinvariant and reve…
bergmeister Nov 12, 2018
079b98d
finish with comments
bergmeister Nov 12, 2018
bd97fcb
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
bergmeister Dec 15, 2018
6d28bcc
Use OrdinalIgnoreCase
bergmeister Dec 15, 2018
7ffbf65
revert accidental change from 2 commits before
bergmeister Dec 15, 2018
9ae39ba
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
bergmeister Jan 9, 2019
e6f4ca3
trigger ci
bergmeister Jan 10, 2019
eae7ff0
Merge branch 'development' into CultureFixes
bergmeister Mar 2, 2019
50e6087
Apply suggestions from code review
rjmholt Mar 5, 2019
7540baf
Merge branch 'development' of https://github.com/PowerShell/PSScriptA…
bergmeister Mar 5, 2019
db4af7b
Use else-less approach to address PR comment
bergmeister Mar 5, 2019
e1752bc
Merge branch 'CultureFixes' of https://github.com/bergmeister/PSScrip…
bergmeister Mar 5, 2019
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
2 changes: 1 addition & 1 deletion Engine/Generic/ModuleDependencyHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ public ModuleDependencyHandler(
? "PSScriptAnalyzer"
: pssaAppDataPath);

modulesFound = new Dictionary<string, PSObject>();
modulesFound = new Dictionary<string, PSObject>(StringComparer.OrdinalIgnoreCase);

// TODO Add PSSA Version in the path
symLinkPath = Path.Combine(pssaAppDataPath, symLinkName);
Expand Down
115 changes: 52 additions & 63 deletions Engine/Generic/RuleSuppression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,61 +202,56 @@ public RuleSuppression(AttributeAst attrAst, int start, int end)
break;
}

switch (name.ArgumentName.ToLower())
var argumentName = name.ArgumentName;
if (argumentName.Equals("rulename", StringComparison.OrdinalIgnoreCase))
{
case "rulename":
if (!String.IsNullOrWhiteSpace(RuleName))
{
Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name);
}

RuleName = (name.Argument as StringConstantExpressionAst).Value;
goto default;

case "rulesuppressionid":
if (!String.IsNullOrWhiteSpace(RuleSuppressionID))
{
Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name);
}

RuleSuppressionID = (name.Argument as StringConstantExpressionAst).Value;
goto default;

case "scope":
if (!String.IsNullOrWhiteSpace(Scope))
{
Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name);
}

Scope = (name.Argument as StringConstantExpressionAst).Value;

if (!scopeSet.Contains(Scope))
{
Error = Strings.WrongScopeArgumentSuppressionAttributeError;
}
if (!String.IsNullOrWhiteSpace(RuleName))
{
Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name);
}

goto default;
RuleName = (name.Argument as StringConstantExpressionAst).Value;
}
else if (argumentName.Equals("rulesuppressionid", StringComparison.OrdinalIgnoreCase))
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot of else ifs in this method and I had to read all of them to check that some kind of state isn't being manipulated by all of them. Would it be easier to use return?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, one would have to move it into a function and other variables would need to be passed to it and there is no else, meaning the caller of it would need an if (returnValue != null) check. I don't think the juice is worth the squeeze here, especially for code that is not likely to be changed and this change is not adding any functionality that would justify such a refactor (any refactor also comes with the risk of a regression, I am not saying we should not change in general, but there seems to be little benefit to me here). Even if previous code is not the best, as long as it does not appear to be dangerously bad, I'd not change it too much in this instance

{
if (!String.IsNullOrWhiteSpace(RuleName))
{
Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name);
}

case "target":
if (!String.IsNullOrWhiteSpace(Target))
{
Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name);
}
RuleName = (name.Argument as StringConstantExpressionAst).Value;
}
else if (argumentName.Equals("scope", StringComparison.OrdinalIgnoreCase))
{
if (!String.IsNullOrWhiteSpace(Scope))
{
Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name);
}

Target = (name.Argument as StringConstantExpressionAst).Value;
goto default;
Scope = (name.Argument as StringConstantExpressionAst).Value;

case "justification":
if (!String.IsNullOrWhiteSpace(Justification))
{
Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name);
}
if (!scopeSet.Contains(Scope))
{
Error = Strings.WrongScopeArgumentSuppressionAttributeError;
}
}
else if (argumentName.Equals("target", StringComparison.OrdinalIgnoreCase))
{
if (!String.IsNullOrWhiteSpace(Target))
{
Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name);
}

Justification = (name.Argument as StringConstantExpressionAst).Value;
goto default;
Target = (name.Argument as StringConstantExpressionAst).Value;
}
else if (argumentName.Equals("justification", StringComparison.OrdinalIgnoreCase))
{
if (!String.IsNullOrWhiteSpace(Justification))
{
Error = String.Format(Strings.NamedAndPositionalArgumentsConflictError, name);
}

default:
break;
Justification = (name.Argument as StringConstantExpressionAst).Value;
}
}
}
Expand Down Expand Up @@ -354,23 +349,17 @@ public static List<RuleSuppression> GetSuppressions(IEnumerable<AttributeAst> at
Regex reg = new Regex(String.Format("^{0}$", ruleSupp.Target.Replace(@"*", ".*")), RegexOptions.IgnoreCase);
IEnumerable<Ast> targetAsts = null;

switch (ruleSupp.Scope.ToLower())
var scope = ruleSupp.Scope;
if (scope.Equals("function", StringComparison.OrdinalIgnoreCase))
{
case "function":
targetAsts = scopeAst.FindAll(item => item is FunctionDefinitionAst && reg.IsMatch((item as FunctionDefinitionAst).Name), true);
goto default;

#if !(PSV3||PSV4)

case "class":
targetAsts = scopeAst.FindAll(item => item is TypeDefinitionAst && reg.IsMatch((item as TypeDefinitionAst).Name), true);
goto default;

#endif

default:
break;
targetAsts = scopeAst.FindAll(ast => ast is FunctionDefinitionAst && reg.IsMatch((ast as FunctionDefinitionAst).Name), true);
}
#if !(PSV3 || PSV4)
else if (scope.Equals("class", StringComparison.OrdinalIgnoreCase))
{
targetAsts = scopeAst.FindAll(ast => ast is TypeDefinitionAst && reg.IsMatch((ast as TypeDefinitionAst).Name), true);
}
#endif

if (targetAsts != null)
{
Expand Down
140 changes: 67 additions & 73 deletions Engine/ScriptAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,20 +283,19 @@ private bool AddProfileItem(
Debug.Assert(includeRuleList != null);
Debug.Assert(excludeRuleList != null);

switch (key.ToLower())
if (key.Equals("severity", StringComparison.OrdinalIgnoreCase))
{
case "severity":
severityList.AddRange(values);
break;
case "includerules":
includeRuleList.AddRange(values);
break;
case "excluderules":
excludeRuleList.AddRange(values);
break;
default:
return false;
severityList.AddRange(values);
Copy link
Contributor

Choose a reason for hiding this comment

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

It strikes me that it would be simpler to just return true here and in the next clause and just return false at the end by default

Copy link
Collaborator Author

@bergmeister bergmeister Mar 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

yes, I agree, I''ve addressed that in this commit

}
else if (key.Equals("includerules", StringComparison.OrdinalIgnoreCase))
{
includeRuleList.AddRange(values);
}
else
{
return false;
}

return true;
}

Expand Down Expand Up @@ -510,75 +509,70 @@ private bool ParseProfileHashtable(Hashtable profile, PathIntrinsics path, IOutp
settings.Keys.CopyTo(settingsKeys, 0);
foreach (var settingKey in settingsKeys)
{
var key = settingKey.ToLower();
object value = settings[key];
switch (key)
{
case "severity":
case "includerules":
case "excluderules":
// value must be either string or collections of string or array
if (value == null
|| !(value is string
|| value is IEnumerable<string>
|| value.GetType().IsArray))
{
writer.WriteError(
new ErrorRecord(
new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongValueHashTable, value, key)),
Strings.WrongConfigurationKey,
ErrorCategory.InvalidData,
profile));
hasError = true;
break;
}
List<string> values = new List<string>();
if (value is string)
{
values.Add(value as string);
}
else if (value is IEnumerable<string>)
{
values.Union(value as IEnumerable<string>);
}
else if (value.GetType().IsArray)
{
// for array case, sometimes we won't be able to cast it directly to IEnumerable<string>
foreach (var val in value as IEnumerable)
{
if (val is string)
{
values.Add(val as string);
}
else
{
writer.WriteError(
new ErrorRecord(
new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongValueHashTable, val, key)),
Strings.WrongConfigurationKey,
ErrorCategory.InvalidData,
profile));
hasError = true;
break;
}
}
}
AddProfileItem(key, values, severityList, includeRuleList, excludeRuleList);
settings[key] = values;
break;
object value = settings[settingKey];

case "rules":
break;

default:
if (settingKey.Equals("severity", StringComparison.OrdinalIgnoreCase) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to suggest changes here, but I keep thinking about the rest of the file so I might open a PR instead

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to defer this. The code works for now, but I'd really like to work on boiling down some common functionality eventually -- something we can work out in the future.

settingKey.Equals("includerules", StringComparison.OrdinalIgnoreCase) ||
settingKey.Equals("excluderules", StringComparison.OrdinalIgnoreCase))
{
// value must be either string or collections of string or array
if (value == null
|| !(value is string
|| value is IEnumerable<string>
|| value.GetType().IsArray))
{
writer.WriteError(
new ErrorRecord(
new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongKeyHashTable, key)),
new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongValueHashTable, value, settingKey)),
Strings.WrongConfigurationKey,
ErrorCategory.InvalidData,
profile));
hasError = true;
break;
}
var values = new List<string>();
if (value is string)
{
values.Add(value as string);
}
else if (value is IEnumerable<string>)
{
values.Union(value as IEnumerable<string>);
}
else if (value.GetType().IsArray)
{
// for array case, sometimes we won't be able to cast it directly to IEnumerable<string>
foreach (var val in value as IEnumerable)
{
if (val is string)
{
values.Add(val as string);
}
else
{
writer.WriteError(
new ErrorRecord(
new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongValueHashTable, val, settingKey)),
Strings.WrongConfigurationKey,
ErrorCategory.InvalidData,
profile));
hasError = true;
break;
}
}
}
AddProfileItem(settingKey, values, severityList, includeRuleList, excludeRuleList);
settings[settingKey] = values;
}
else if (settingKey.Equals("excluderules", StringComparison.OrdinalIgnoreCase))
{
writer.WriteError(
new ErrorRecord(
new InvalidDataException(string.Format(CultureInfo.CurrentCulture, Strings.WrongKeyHashTable, settingKey)),
Strings.WrongConfigurationKey,
ErrorCategory.InvalidData,
profile));
hasError = true;
}
}
return hasError;
Expand Down
4 changes: 2 additions & 2 deletions Engine/Settings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ private void parseSettingsHashtable(Hashtable settingsHashtable)
var settings = GetDictionaryFromHashtable(settingsHashtable);
foreach (var settingKey in settings.Keys)
{
var key = settingKey.ToLower();
var key = settingKey.ToLowerInvariant(); // ToLowerInvariant is important to also work with turkish culture, see https://github.com/PowerShell/PSScriptAnalyzer/issues/1095
object val = settings[key];
switch (key)
{
Expand Down Expand Up @@ -514,7 +514,7 @@ private static object GetSafeValueFromExpressionAst(ExpressionAst exprAst)

case VariableExpressionAst varExprAst:
// $true and $false are VariableExpressionAsts, so look for them here
switch (varExprAst.VariablePath.UserPath.ToLower())
switch (varExprAst.VariablePath.UserPath.ToLowerInvariant())
{
case "true":
return true;
Expand Down
8 changes: 4 additions & 4 deletions PSCompatibilityAnalyzer/Microsoft.PowerShell.CrossCompatibility/Utility/PlatformNaming.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ public static string GetPlatformName(PlatformData platform)
{
string psVersion = platform.PowerShell.Version?.ToString();
string osVersion = platform.OperatingSystem.Version;
string osArch = platform.OperatingSystem.Architecture.ToString().ToLower();
string pArch = platform.PowerShell.ProcessArchitecture.ToString().ToLower();
string dotnetVersion = platform.Dotnet.ClrVersion.ToString().ToLower();
string dotnetEdition = platform.Dotnet.Runtime.ToString().ToLower();
string osArch = platform.OperatingSystem.Architecture.ToString().ToLowerInvariant();
string pArch = platform.PowerShell.ProcessArchitecture.ToString().ToLowerInvariant();
string dotnetVersion = platform.Dotnet.ClrVersion.ToString().ToLowerInvariant();
string dotnetEdition = platform.Dotnet.Runtime.ToString().ToLowerInvariant();

string[] platformNameComponents;
switch (platform.OperatingSystem.Family)
Expand Down
2 changes: 1 addition & 1 deletion Rules/ProvideCommentHelp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ public override string ToString()
public virtual string ToString(int? tabStop)
{
var sb = new StringBuilder();
sb.Append(".").AppendLine(Name.ToUpper());
sb.Append(".").AppendLine(Name.ToUpperInvariant()); // ToUpperInvariant is important to also work with turkish culture, see https://github.com/PowerShell/PSScriptAnalyzer/issues/1095
if (!String.IsNullOrWhiteSpace(Description))
{
sb.Append(Snippetify(tabStop, Description));
Expand Down
2 changes: 1 addition & 1 deletion Rules/UseShouldProcessCorrectly.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ public override bool Equals(Object other)
/// </summary>
public override int GetHashCode()
{
return name.ToLower().GetHashCode();
return name.ToLowerInvariant().GetHashCode();
}
}

Expand Down
2 changes: 1 addition & 1 deletion ScriptRuleDocumentation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ function Measure-RequiresRunAsAdministrator
if ($Ast -is [System.Management.Automation.Language.AssignmentStatementAst])
{
[System.Management.Automation.Language.AssignmentStatementAst]$asAst = $Ast
if ($asAst.Right.ToString().ToLower() -eq '[system.security.principal.windowsbuiltinrole]::administrator')
if ($asAst.Right.ToString() -eq '[system.security.principal.windowsbuiltinrole]::administrator')
{
$returnValue = $true
}
Expand Down
Loading