PSScriptAnalyzer fixes by category #4261

bergmeister · 2017-07-14T23:47:18Z

This is the updated version of PR #4213, in which it was requested to split the commits on a per kind category.
The intend is to help with fixing #3791. The fixed PSScriptAnalyzer warnings are PSPossibleIncorrectComparisonWithNull and PSAvoidUsingCmdletAliases for ForEach-Object, Where-Object and Select-Object since those warning types were responsible for the majority of warnings.

…ForEach-Object' (alias is '%' or 'foreach')

…Where-Object' (alias is '?' or 'where')

…Select-Object' (alias is 'select')

…thNull. Essentially, $null has to be on the left hand side when using it for comparison. A Test in ParameterBinding.Tests.ps1 needed adapting as this test used to rely on the wrong null comparison It was also suggested in the initial PR PowerShell#4205 to replace a subset of tests of kind '($object -eq $null) | Should Be $true' with '($null -eq $object) | Should BeNullOrEmpty'

msftclas · 2017-07-14T23:47:21Z

@bergmeister,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

daxian-dbw · 2017-07-15T01:03:47Z

@bergmeister Thank you so much for taking extra effort splitting the original PR by the fix categories!

iSazonov · 2017-07-15T19:03:40Z

build.psm1

@@ -668,7 +668,7 @@ function Get-PesterTag {
    $alltags = @{}
    $warnings = @()

-    get-childitem -Recurse $testbase -File |?{$_.name -match "tests.ps1"}| ForEach-Object {
+    get-childitem -Recurse $testbase -File |Where-Object {$_.name -match "tests.ps1"}| ForEach-Object {


Please add a space before Where-Object and after "tests.ps1"}

iSazonov · 2017-07-15T19:05:54Z

demos/Apache/apache-demo.ps1

@@ -2,7 +2,7 @@ Import-Module $PSScriptRoot/Apache/Apache.psm1

 #list Apache Modules
 Write-Host -Foreground Blue "Get installed Apache Modules like *proxy* and Sort by name"
-Get-ApacheModule |Where {$_.ModuleName -like "*proxy*"}|Sort-Object ModuleName | Out-Host
+Get-ApacheModule | Where-Object {$_.ModuleName -like "*proxy*"}|Sort-Object ModuleName | Out-Host


Please add spaces in "*proxy*"}|Sort-Object.

iSazonov · 2017-07-15T19:11:29Z

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

    {{
        $script:PSSession = $PSSession

-        if ($createdByModule -and ($script:PSSession -ne $null))
+        if ($null -ne $createdByModule -and ($script:PSSession))


Please correct.

iSazonov · 2017-07-15T19:33:16Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Implicit.Remoting.Tests.ps1

@@ -417,7 +417,7 @@ Describe "Proxy module is usable when the original runspace is no longer around"

        It "Verifies Remove-Module removed the runspace that was automatically created" -Pending {
            Remove-Module $module -Force
-            ((Get-PSSession -InstanceId $internalSession.InstanceId -ErrorAction SilentlyContinue) -eq $null) | Should Be $true
+            ($null -eq (Get-PSSession -InstanceId $internalSession.InstanceId -ErrorAction SilentlyContinue)) | Should Be $true


Please change to Should BeNullOrEmpty.

iSazonov · 2017-07-15T19:34:22Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Implicit.Remoting.Tests.ps1

@@ -452,7 +452,7 @@ Describe "Proxy module is usable when the original runspace is no longer around"
        # removing the module should remove the implicitly/magically created runspace
        It "Verifies Remove-Module removes automatically created runspace" -Pending {
            Remove-Module $module -Force
-            ((Get-PSSession -InstanceId $internalSession.InstanceId -ErrorAction SilentlyContinue) -eq $null) | Should Be $true
+            ($null -eq (Get-PSSession -InstanceId $internalSession.InstanceId -ErrorAction SilentlyContinue)) | Should Be $true


Please change to Should BeNullOrEmpty.

iSazonov · 2017-07-15T19:35:06Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Implicit.Remoting.Tests.ps1

@@ -767,7 +767,7 @@ Describe "Import-PSSession functional tests" -tags "Feature" {
    }

    It "Helper functions should not be imported" -Skip:$skipTest {
-        ((Get-Item function:*PSImplicitRemoting* -ErrorAction SilentlyContinue) -eq $null) | Should Be $true
+        ($null -eq (Get-Item function:*PSImplicitRemoting* -ErrorAction SilentlyContinue)) | Should Be $true


Please change to Should BeNullOrEmpty.

iSazonov · 2017-07-15T19:35:21Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Implicit.Remoting.Tests.ps1

@@ -808,11 +808,11 @@ Describe "Import-PSSession functional tests" -tags "Feature" {
        }

        It "Temporary module should be automatically removed after runspace is closed" -Skip:$skipTest {
-            ((Get-Module | Where-Object { $_.Path -eq $module.Path }) -eq $null) | Should Be $true
+            ($null -eq (Get-Module | Where-Object { $_.Path -eq $module.Path })) | Should Be $true


Please change to Should BeNullOrEmpty.

iSazonov · 2017-07-15T19:35:34Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Implicit.Remoting.Tests.ps1

        }

        It "Temporary psm1 file should be automatically removed after runspace is closed" -Skip:$skipTest {
-            ((Get-Item $module.Path -ErrorAction SilentlyContinue) -eq $null) | Should Be $true
+            ($null -eq (Get-Item $module.Path -ErrorAction SilentlyContinue)) | Should Be $true


Please change to Should BeNullOrEmpty.

iSazonov · 2017-07-15T19:36:28Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Implicit.Remoting.Tests.ps1

        }

-        ((Get-Item function:Get-MyVariable -ErrorAction SilentlyContinue) -eq $null) | Should Be $true
+        ($null -eq (Get-Item function:Get-MyVariable -ErrorAction SilentlyContinue)) | Should Be $true


Please change to Should BeNullOrEmpty.

iSazonov · 2017-07-15T19:41:14Z

test/powershell/engine/SemanticVersion.Tests.ps1

@@ -117,9 +117,9 @@ Describe "SemanticVersion api tests" -Tags 'CI' {
            $operand -eq $operand | Should Be $true
            $operand -ne $operand | Should Be $false
            $null -eq $operand | Should Be $false
-            $operand -eq $null | Should Be $false
+            $null -eq $operand | Should Be $false
+            $null -ne $operand | Should Be $true


I believe it is special tests - please revert the changes in the file.

…riptAnalyzer fixes by category): -Spacing in build.psm1 and apache-demo.ps1 -Correction of logical mistake in Implicit.Remoting.Tests.ps1 -Changing more tests from '$null -eq $object | Should Be $true' to '$object | Should BeNullOrEmpty' -Revert a change initially made to SemanticVersion.Tests.ps1

iSazonov · 2017-07-17T06:49:33Z

LGTM.

daxian-dbw · 2017-07-17T17:12:01Z

I don't think we should touch CSharp files and Markdown files. PSScriptAnalyzer warnings should be applicable only to powershell script files.

iSazonov · 2017-07-17T17:19:04Z

Should we fix CSharp files and Markdown files in separate PRs?

daxian-dbw · 2017-07-17T17:29:06Z

I don't think we need to make those changes in non-powershell-script files, since the goal is to avoid PSScriptAnalyzer warnings.
It's easy to revert changes in CSharp/Markdown files -- do a soft reset, and then cd into PowerShell\src and run git checkout .. It should reset all changes in PowerShell\src folder. And it's similar to Markdown files.

bergmeister · 2017-07-19T23:34:37Z

@daxian-dbw If you say that the only goal is to avoid PSScriptAnalyzer warnings, then I'm not sure if you considered the reason why those rules exist. They are there best practices to avoid unintended/wrong behaviour and therefore apply to any file using PS syntax IMHO. PSPossibleIncorrectComparisonWithNull is the best example where it can result in e.g. an if condition that always evaluates to true. I can understand if you are concerned that in the case of PSPossibleIncorrectComparisonWithNull, changes in the code of the actual product could lead to a regression and should be excluded but in the case of PSAvoidUsingCmdletAliases, I do not see a reason as to why cs/md files should be excluded. WDYT?

daxian-dbw · 2017-07-20T06:38:22Z

@bergmeister Thanks for the comment, and you have very good points. Yes, my main concern about changing the .cs files is the potential regression. Given the size of changes, it could be easy for us to overlook something in the review. That being said, I think you are right that the scripts declared in C# code should follow the rules as well, but I would suggest that we leave the comments unchanged in C# code, especially those that still use other aliases.

As for the PSScriptAnalyzer rules, it's very useful to write easy-to-maintain and less error-prone PS script files, but it's not necessarily always applicable to any PSH syntax, for example, the script that serves as a one-liner command for you to run interactively, in which case aliases may be more preferred because they make the command concise and easy to type. The changes in KnownIssues.md would be a good example of it -- ls (gci *.txt | % name) is a command for a user to run interactively, and thus it's OK to keep the aliases. It's a similar idea for scripts in c# comments -- those simple scripts are only for demonstrating the scenarios, and thus it's OK to use the aliases to keep them concise.

iSazonov · 2017-07-20T06:50:18Z

Only we don't type samples from code and md files - we copy-paste 😄

daxian-dbw · 2017-07-20T15:58:38Z

@iSazonov right :) my point is that for the one-liner scripts like those, it's OK to use aliases to keep them concise.
I will do a pass of review on the .cs files.

daxian-dbw · 2017-07-20T17:18:59Z

docs/KNOWNISSUES.md

@@ -51,7 +51,7 @@ Removing the aliases exposes the native command experience to the PowerShell use

 Currently, PowerShell only does wildcard expansion (globbing) for built-in cmdlets on Windows, and for external commands or binaries as well as cmdlets on Linux.
 This means that a command like `ls *.txt` will fail because the asterisk will not be expanded to match file names.
-You can work around this by doing `ls (gci *.txt | % name)` or, more simply, `gci *.txt` using the PowerShell built-in equivalent to `ls`.
+You can work around this by doing `ls (gci *.txt | ForEach-Object name)` or, more simply, `gci *.txt` using the PowerShell built-in equivalent to `ls`.


As we discussed, this change is not necessary. This is a one-line script to be used interactively. It's OK to use aliases to keep it concise.

daxian-dbw · 2017-07-20T17:26:17Z

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

@@ -2138,9 +2138,9 @@ private Assembly LoadAssemblyHelper(string assemblyName)
            // 10,000 iterations of this method takes ~800 ms for a string array, and ~ 200ms for the dictionary
            //
            // $dlls = ((dir c:\windows\Microsoft.NET\Framework\ -fi *.dll -rec) + (dir c:\windows\assembly -fi *.dll -rec)) + (dir C:\Windows\Microsoft.NET\assembly) |
-            //     % { [Reflection.Assembly]::LoadFrom($_.FullName) }
+            //     ForEach-Object { [Reflection.Assembly]::LoadFrom($_.FullName) }


These changes probably should be reverted, as dir is still used here and short form of parameters are also used such as -fi, -rec and -u. It's OK to use aliases in cases like this to make it concise.

daxian-dbw · 2017-07-20T17:26:29Z

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

@@ -1646,7 +1646,7 @@ internal static char SetDelimiter(PSCmdlet Cmdlet, string ParameterSetName, char
                    if (UseCulture == true)
                    {
                        // ListSeparator is apparently always a character even though the property returns a string, checked via:
-                        // [CultureInfo]::GetCultures("AllCultures") | % { ([CultureInfo]($_.Name)).TextInfo.ListSeparator } | ? Length -ne 1
+                        // [CultureInfo]::GetCultures("AllCultures") | ForEach-Object { ([CultureInfo]($_.Name)).TextInfo.ListSeparator } | Where-Object Length -ne 1


daxian-dbw · 2017-07-20T17:53:31Z

src/Microsoft.PowerShell.GraphicalHost/commandHelpers/ShowCommandHelper.cs

@@ -183,7 +183,7 @@ Function GetParameterInfo
                $validateSetAttributes = $parameter.Attributes | Where {
                    [ValidateSet].IsAssignableFrom($_.GetType())
                }
-                if (($validateSetAttributes -ne $null) -and ($validateSetAttributes.Count -gt 0))
+                if (($null -ne $validateSetAttributes) -and ($validateSetAttributes.Count -gt 0))


Here, $validateSetAttributes is an array, but this change is good because of the -and ($validateSetAttributes.Count -gt 0) 😄

Good. Although if I had written the code myself I would have rather declared $validateSetAttributes as [System.Array] because then one would not only avoid the problem that the variable can be null but also prevents gotchas where in the case of array size 1 the type would not be an array and therefore addressing it later as $validateSetAttributes[0] would result in undesired behaviour (in most cases PowerShell would return the first character of the string representation). Just my two cents.

daxian-dbw · 2017-07-20T18:29:35Z

src/Modules/Windows-Full/Microsoft.PowerShell.ODataUtils/Microsoft.PowerShell.ODataAdapter.ps1

@@ -1634,7 +1634,7 @@ function AddParametersCDXML
        [Hashtable] $complexTypeMapping
    )

-    $properties | ? { $_ -ne $null } | % {
+    $properties | Where-Object { $_ -ne $null } | ForEach-Object {


{ $_ -ne $null }

Should be { $null -ne $_ }

daxian-dbw · 2017-07-20T20:11:36Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Implicit.Remoting.Tests.ps1


                ## BadVerb-Variable should be a function, not an alias (1)
-                ((Get-Item Function:\BadVerb-Variable -ErrorAction SilentlyContinue) -ne $null) | Should Be $true
+                Get-Item Function:\BadVerb-Variable -ErrorAction SilentlyContinue | Should BeNullOrEmpty


daxian-dbw · 2017-07-20T20:11:43Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Implicit.Remoting.Tests.ps1


                ## BadVerb-Variable should be a function, not an alias (2)
-                ((Get-Item Alias:\BadVerb-Variable -ErrorAction SilentlyContinue) -eq $null) | Should Be $true
+                Get-Item Alias:\BadVerb-Variable -ErrorAction SilentlyContinue | Should BeNullOrEmpty


daxian-dbw · 2017-07-20T20:12:01Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Implicit.Remoting.Tests.ps1


                ## BadVerb-Variable should be a function, not an alias (2)
-                ((Get-Item Alias:\BadVerb-Variable -ErrorAction SilentlyContinue) -eq $null) | Should Be $true
+                Get-Item Alias:\BadVerb-Variable -ErrorAction SilentlyContinue | Should BeNullOrEmpty


daxian-dbw · 2017-07-20T20:12:17Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Implicit.Remoting.Tests.ps1


                ## BadVerb-Variable should be an alias, not a function (2)
-                ((Get-Item Alias:\BadVerb-Variable -ErrorAction SilentlyContinue) -ne $null) | Should Be $true
+                Get-Item Alias:\BadVerb-Variable -ErrorAction SilentlyContinue | Should BeNullOrEmpty


daxian-dbw · 2017-07-20T20:16:39Z

test/powershell/engine/Remoting/InvokeCommandRemoteDebug.Tests.ps1

-            if ($rs2 -ne $null) { $rs2.Dispose() }
-            if ($remoteSession -ne $null) { Remove-PSSession $remoteSession -ErrorAction SilentlyContinue }
+            if ($null -ne $testDebugger) { $testDebugger.Release() }
+            if ($null -ne $psl) { $ps.Dispose() }


($null -ne $psl)

Should be $ps

daxian-dbw · 2017-07-20T20:23:38Z

@bergmeister Please push new commits to address the comments and resolve conflicts. Please DO NOT rebase your branch. This way, we can keep the review history which is very important for large size changes like this PR.

daxian-dbw · 2017-07-21T16:50:46Z

@bergmeister I just resolved the 2 conflicts. I think we need faster review iterations on this PR, because there are many other PRs changing the script files covered in this PR and thus there will be more conflicts very soon.

@daxian-dbw

…4261 by @daxian-dbw. This includes a revert of a logical error in Microsoft.PowerShell.ODataV4Adapter.ps1 and reverts back some functions to their alias where the reviewer felt that they were still appropriate. Some minor cosmetic changes and test syntax improvements.

bergmeister · 2017-07-21T22:27:27Z

@daxian-dbw I have applied your requested changes.

daxian-dbw

LGTM. Thanks @bergmeister!

iSazonov · 2017-07-22T19:40:04Z

@bergmeister Thank you for your patience and hard work!

bergmeister · 2017-07-22T21:41:35Z

@iSazonov @daxian-dbw Thank you both as well. I really had no idea it would turn out to be that complex. I am currently developing a -AutoFix switch for Invoke-PSScriptAnalyzer that will make it easier for everyone to fix some of the remaining warnings in the future.

bergmeister added 4 commits July 13, 2017 22:10

Fix PSScriptAnalyzer warnings of type PSAvoidUsingCmdletAliases for '…

7dd62e8

…ForEach-Object' (alias is '%' or 'foreach')

Fix PSScriptAnalyzer warnings of type PSAvoidUsingCmdletAliases for '…

9d6e3aa

…Where-Object' (alias is '?' or 'where')

Fix PSScriptAnalyzer warnings of type PSAvoidUsingCmdletAliases for '…

b3f8123

…Select-Object' (alias is 'select')

msftclas added the cla-already-signed label Jul 14, 2017

bergmeister mentioned this pull request Jul 14, 2017

Fixes of around 350 PSScriptAnalyzer warnings #4213

Closed

daxian-dbw self-assigned this Jul 15, 2017

daxian-dbw requested review from daxian-dbw and iSazonov July 15, 2017 01:03

iSazonov reviewed Jul 15, 2017

View reviewed changes

daxian-dbw reviewed Jul 20, 2017

View reviewed changes

Merge branch 'master' into FixPSScriptAnalyzerWarningsByCategory

f07469c

daxian-dbw added 3 commits July 21, 2017 19:17

Change 'dir' to 'Get-ChildItem'

3302a02

Revert the 2 changes in comments in powershell.cs

ceb9286

Revert changes in comments in PSCommand.cs

de488ed

Change to 'Should Be $null' in LocalUser.Tests.ps1

7985744

daxian-dbw approved these changes Jul 22, 2017

View reviewed changes

daxian-dbw merged commit ffd39b2 into PowerShell:master Jul 22, 2017

daxian-dbw mentioned this pull request Jul 24, 2017

Fix regressions that cause implicit remoting tests to fail #4326

Merged

PSScriptAnalyzer fixes by category #4261

PSScriptAnalyzer fixes by category #4261

Conversation

bergmeister commented Jul 14, 2017

msftclas commented Jul 14, 2017

daxian-dbw commented Jul 15, 2017

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 Jul 17, 2017

daxian-dbw commented Jul 17, 2017

iSazonov commented Jul 17, 2017

daxian-dbw commented Jul 17, 2017 • edited Loading

bergmeister commented Jul 19, 2017

daxian-dbw commented Jul 20, 2017 • edited Loading

iSazonov commented Jul 20, 2017

daxian-dbw commented Jul 20, 2017

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

bergmeister Jul 21, 2017 • edited Loading

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

daxian-dbw commented Jul 20, 2017 • edited Loading

daxian-dbw commented Jul 21, 2017

bergmeister commented Jul 21, 2017

daxian-dbw left a comment

Choose a reason for hiding this comment

iSazonov commented Jul 22, 2017

bergmeister commented Jul 22, 2017

daxian-dbw commented Jul 17, 2017 •

edited

Loading

daxian-dbw commented Jul 20, 2017 •

edited

Loading

bergmeister Jul 21, 2017 •

edited

Loading

daxian-dbw commented Jul 20, 2017 •

edited

Loading