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

bergmeister · 2018-11-08T20:11:20Z

PR Summary

Fixes Not all tests pass using non-English culture (e.g. Turkish, Korean, German) #1098 by specifying the culture in CommunityAnalyzerRules when calling Import-LocalizedData from a culture that is not en-US. This is a PowerShell Core bug, I also raised an issue that I reference in the comment
Fixes An exception is thrown when CurrentCulture is Turkish (tr-TR) #1095 where calls to ToLower() or ToUpper can return a special Turkish i character that then does not pass the comparison when compared to a normal i. The approach is to eliminate the need for it by using culture invariant comparison where possible and otherwise calling ToLowerInvarian().
Remove redundant ToLower calls in PowerShell since PowerShell is case insensitive

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

…re repo)

…rt change where it is not needed

JamesWTruher

in PowerShell, we've been instructed to use StringComparer.OrdinalIgnoreCase. I'm thinking that we should do that here as well unless there's a really good reason not to.
http://msdn.microsoft.com/en-us/library/system.stringcomparer.ordinalignorecase.aspx
http://msdn.microsoft.com/en-us/library/system.stringcomparer.invariantcultureignorecase.aspx

…nalyzer into CultureFixes # Conflicts: # Engine/Generic/ModuleDependencyHandler.cs

bergmeister · 2018-12-15T13:03:52Z

Ok, I've made it use OrdinalIgnoreCase instead everywhere

…nalyzer into CultureFixes

fix also ToLower call in new compatibility module # Conflicts: # Engine/ScriptAnalyzer.cs # Engine/Settings.cs # tools/appveyor.psm1

rjmholt

LGTM

Engine/Generic/RuleSuppression.cs

rjmholt · 2019-03-05T19:34:36Z

Engine/Generic/RuleSuppression.cs

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


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?

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

Engine/Generic/RuleSuppression.cs

rjmholt · 2019-03-05T19:36:58Z

Engine/ScriptAnalyzer.cs

-                    break;
-                default:
-                    return false;
+                severityList.AddRange(values);


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

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

rjmholt · 2019-03-05T19:50:58Z

Engine/ScriptAnalyzer.cs

-                        break;
-
-                    default:
+                if (settingKey.Equals("severity", StringComparison.OrdinalIgnoreCase) ||


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

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.

Co-Authored-By: bergmeister <[email protected]>

…nalyzer into CultureFixes

…tAnalyzer into CultureFixes

JamesWTruher

lgtm

…erShell#1099) * Fix Parsing of settings that occurs when using turkish culture * use german culture before running tests to exhibit failures * set ui culture as well * fix communityanalyzer for turkish culture * fix ProvideCommentHelp for turkish culture (i problem again) * update comments * fix for import-localizedDatabug in pscore (opened issue 8219 in ps core repo) * change to UI culture * use invariant culture comparison instead of tolowerinvariant and revert change where it is not needed * finish with comments * Use OrdinalIgnoreCase * revert accidental change from 2 commits before * trigger ci * Apply suggestions from code review Co-Authored-By: bergmeister <[email protected]> * Use else-less approach to address PR comment

bergmeister added 5 commits November 6, 2018 19:56

Fix Parsing of settings that occurs when using turkish culture

010a862

use german culture before running tests to exhibit failures

57ab21c

set ui culture as well

dcfe1cf

fix communityanalyzer for turkish culture

5fea008

fix ProvideCommentHelp for turkish culture (i problem again)

a8df05d

bergmeister self-assigned this Nov 8, 2018

bergmeister mentioned this pull request Nov 8, 2018

Not all tests pass using non-English culture (e.g. Turkish, Korean, German) #1098

Closed

bergmeister added 5 commits November 8, 2018 21:17

update comments

c33c38b

fix for import-localizedDatabug in pscore (opened issue 8219 in ps co…

479ed3a

…re repo)

change to UI culture

227b64f

use invariant culture comparison instead of tolowerinvariant and reve…

cee22f9

…rt change where it is not needed

finish with comments

079b98d

bergmeister changed the title ~~WIP More fixes for turkish culture~~ Fixes PSSA for Turkish culture and tests when culture is not en-US Nov 12, 2018

bergmeister changed the title ~~Fixes PSSA for Turkish culture and tests when culture is not en-US~~ Fix PSSA for Turkish culture and tests when culture is not en-US Nov 12, 2018

bergmeister requested a review from JamesWTruher November 12, 2018 18:34

bergmeister added the Issue - Bug label Nov 12, 2018

bergmeister added this to the 1.18 milestone Nov 12, 2018

bergmeister mentioned this pull request Nov 12, 2018

Fix culture issue error that occurs when using turkish culture and Invoke-Formatter #1097

Closed

5 tasks

JamesWTruher suggested changes Dec 6, 2018

View reviewed changes

bergmeister added 3 commits December 15, 2018 12:14

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

bd97fcb

…nalyzer into CultureFixes # Conflicts: # Engine/Generic/ModuleDependencyHandler.cs

Use OrdinalIgnoreCase

6d28bcc

revert accidental change from 2 commits before

7ffbf65

bergmeister added 2 commits January 9, 2019 21:23

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

9ae39ba

…nalyzer into CultureFixes

trigger ci

e6f4ca3

bergmeister added the (Re-)Review Needed Feedback has been addressed during PR stage or is required in the first place. label Jan 29, 2019

Merge branch 'development' into CultureFixes

eae7ff0

fix also ToLower call in new compatibility module # Conflicts: # Engine/ScriptAnalyzer.cs # Engine/Settings.cs # tools/appveyor.psm1

rjmholt approved these changes Mar 5, 2019

View reviewed changes

rjmholt and others added 2 commits March 5, 2019 20:33

Apply suggestions from code review

50e6087

Co-Authored-By: bergmeister <[email protected]>

Merge branch 'development' of https://github.com/PowerShell/PSScriptA…

7540baf

…nalyzer into CultureFixes

bergmeister added 2 commits March 5, 2019 20:49

Use else-less approach to address PR comment

db4af7b

Merge branch 'CultureFixes' of https://github.com/bergmeister/PSScrip…

e1752bc

…tAnalyzer into CultureFixes

bergmeister requested a review from JamesWTruher March 5, 2019 21:41

JamesWTruher approved these changes Mar 7, 2019

View reviewed changes

bergmeister merged commit a68b139 into PowerShell:development Mar 8, 2019

bergmeister mentioned this pull request May 6, 2020

Add beginnings of PSScriptAnalyzer 2.0 #1476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

bergmeister commented Nov 8, 2018 •

edited

Loading

Uh oh!

JamesWTruher left a comment

Uh oh!

bergmeister commented Dec 15, 2018

Uh oh!

rjmholt left a comment

Uh oh!

Uh oh!

rjmholt Mar 5, 2019

Uh oh!

bergmeister Mar 5, 2019

Uh oh!

Uh oh!

rjmholt Mar 5, 2019

Uh oh!

bergmeister Mar 5, 2019 •

edited

Loading

Uh oh!

rjmholt Mar 5, 2019

Uh oh!

rjmholt Mar 5, 2019

Uh oh!

JamesWTruher left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

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

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

Uh oh!

Conversation

bergmeister commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

JamesWTruher left a comment

Choose a reason for hiding this comment

Uh oh!

bergmeister commented Dec 15, 2018

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjmholt Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

bergmeister Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjmholt Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

bergmeister Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

rjmholt Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

JamesWTruher left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bergmeister commented Nov 8, 2018 •

edited

Loading

bergmeister Mar 5, 2019 •

edited

Loading