Fix PSScriptAnalyzer warnings #4205

bergmeister · 2017-07-09T22:05:42Z

To help fixing #3791
Fixes a few hundred warnings of type PSPossibleIncorrectComparisonWithNull, PSAvoidUsingCmdletAliases (mostly for ForEach-Object, Where-Object, Select-Object) and some PSUseDeclaredVarsMoreThanAssignment warnings.
As part of this, it was also found that a test in ParameterBindingTests needed improvement because since the null comparison is now correct, the test failed initially.

… Where-Object (?), Select-Object and a few others. Fixed some PSUseDeclaredVarsMoreThanAssignment warnings (one must be careful with them as some of those warnings are false positives)

…omparison is now correct and also removed unnecessary initialisation that was not needed for the test but made it difficult to know what was going. Enhanced the test. Fixed one typo in Dynamicparameters.Tests.ps1 that got introduced during the last refactoring and made 2 tests fail.

msftclas · 2017-07-09T22:05:47Z

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

iSazonov

As requested in #3791 we should add meta test with scriptanalyzer.

iSazonov · 2017-07-10T03:36:42Z

build.psm1

    $IsFedora = $LinuxInfo.ID -match 'fedora' -and $LinuxInfo.VERSION_ID -ge 24
    $IsOpenSUSE = $LinuxInfo.ID -match 'opensuse'
-    $IsOpenSUSE13 = $IsOpenSUSE -and $LinuxInfo.VERSION_ID  -match '13'
-    ${IsOpenSUSE42.1} = $IsOpenSUSE -and $LinuxInfo.VERSION_ID  -match '42.1'


The variables is unused - so Ok to remove.

iSazonov · 2017-07-10T03:38:35Z

build.psm1

                    if ($count -eq 0)
                    {
-                        sleep 1
+                        Start-Sleep 1


Maybe add a parameter name.

iSazonov · 2017-07-10T03:49:37Z

demos/Azure/Azure-Demo.ps1

 ### Install-Package -Name AzureRM.NetCore.Preview -Source https://www.powershellgallery.com/api/v2 -ProviderName NuGet -ExcludeVersion -Destination <Folder you want this to be installed>
 ###
-### Ensure $env:PSModulePath is updated with the location you used to install.
+### Ensure $env:PSMODULEPATH is updated with the location you used to install.


We should use "PSModulePath" - see PowerShell-Committee conclusion #3227 (comment)

iSazonov · 2017-07-10T03:58:57Z

src/Microsoft.PowerShell.Workflow.ServiceCore/ServiceCore/WorkflowCore/ImportWorkflowCommand.cs

                }}

-                if (-not $AsJob -and $job -ne $null)
+                if ($null -ne -not $AsJob -and $job)


Corrupted logic. Please see operator precedence

iSazonov · 2017-07-10T04:11:11Z

...Modules/Windows-Full/Microsoft.PowerShell.ODataUtils/Microsoft.PowerShell.ODataV4Adapter.ps1

 		$xmlWriter = SaveCDXMLHeader $xmlWriter $Uri $singletonName $singletonName $CmdletAdapter

-        if ($associatedEntityType.BaseType -eq $null -and $associatedEntityType.BaseTypeStr -ne $null -and $associatedEntityType.BaseTypeStr -ne '')
+        if ($null -eq $associatedEntityType.BaseTyp -and $null -ne $associatedEntityType.BaseTypeStr -and $associatedEntityType.BaseTypeStr -ne '')


Lost char: $associatedEntityType.BaseTyp

iSazonov · 2017-07-10T04:58:00Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Select-Object.Tests.ps1


    It "Should be able to use the alias" {
-	{ $dirObject | select } | Should Not Throw
+	{ $dirObject | Select-Object} | Should Not Throw


Please revert the change.

iSazonov · 2017-07-10T04:58:53Z

test/powershell/Modules/PackageManagement/PackageManagement.Tests.ps1

        $gpp = Get-PackageProvider

-        $gpp | ?{ $_.name -eq "NuGet" } | should not BeNullOrEmpty
+        $gpp | Where-Object{ $_.name -eq "NuGet" } | should not BeNullOrEmpty


Please add space after Where-Object.

iSazonov · 2017-07-10T04:59:05Z

test/powershell/Modules/PackageManagement/PackageManagement.Tests.ps1

+        $gpp | Where-Object{ $_.name -eq "NuGet" } | should not BeNullOrEmpty

-        $gpp | ?{ $_.name -eq "PowerShellGet" } | should not BeNullOrEmpty
+        $gpp | Where-Object{ $_.name -eq "PowerShellGet" } | should not BeNullOrEmpty


Please add space after Where-Object.

iSazonov · 2017-07-10T05:08:38Z

test/tools/OpenCover/OpenCover.psm1

-            $c1 = $r1.Assembly.ClassCoverage | ?{$_.ClassName -eq $Class }
-            $c2 = $r2.Assembly.ClassCoverage | ?{$_.ClassName -eq $Class }
+            $c1 = $r1.Assembly.ClassCoverage | Where-Object{$_.ClassName -eq $Class }
+            $c2 = $r2.Assembly.ClassCoverage | Where-Object{$_.ClassName -eq $Class }


Please add space after Where-Object in both lines.

iSazonov · 2017-07-10T05:09:25Z

test/tools/OpenCover/OpenCover.psm1

    }
    $CoverageData.PSTypeNames.Insert(0,"OpenCover.CoverageData")
-    Add-Member -InputObject $CoverageData -MemberType ScriptMethod -Name GetClassCoverage -Value { param ( $name ) $this.assembly.classcoverage | ?{$_.classname -match $name } }
+    Add-Member -InputObject $CoverageData -MemberType ScriptMethod -Name GetClassCoverage -Value { param ( $name ) $this.assembly.classcoverage | Where-Object{$_.classname -match $name } }


Please add space after Where-Object.

…ludes spaces after Where-Object/Select-Object, 4 lines being reverted due to accidental changes and minor test refactoring. The test refactoring idea was basically to change '$null -eq $something | Should Be $true' to '$something | Should BeNullOrEmpty'

bergmeister · 2017-07-10T07:02:51Z

Thanks @iSazonov for taking such a detailed look and spotting the accidental mistakes that I made when going through the hundreds of refactorings. I have followed all your suggestions and submitted the fix as you can see above. The total number of warnings seems to have gone down by about 350.

iSazonov · 2017-07-10T07:31:47Z

src/System.Management.Automation/engine/Types_Ps1Xml.cs

            td24.Members.Add("IPV4Address",
                new ScriptPropertyData(@"IPV4Address", GetScriptBlock(@"$iphost = [System.Net.Dns]::GetHostEntry($this.address)
-          $iphost.AddressList | Where-Object{ $_.AddressFamily -eq [System.Net.Sockets.AddressFamily]::InterNetwork } | Select-Object -first 1"), null));
+          $iphost.AddressList | Where-Object{  $_.AddressFamily -eq [System.Net.Sockets.AddressFamily]::InterNetwork } | Select-Object -first 1"), null));


Please remove extra space before $_ and add space after Where-Object and before {.

The same typo is below.

iSazonov · 2017-07-10T07:34:28Z

test/powershell/Modules/Microsoft.PowerShell.Security/AclCmdlets.Tests.ps1


        $newacl = get-acl $directory
-        $newrule = $newacl.Access | Where-Object{ $accessrule.FileSystemRights -eq $_.FileSystemRights -and $accessrule.AccessControlType -eq $_.AccessControlType -and $accessrule.IdentityReference -eq $_.IdentityReference }
+        $newrule = $newacl.Access | Where-Object{  $accessrule.FileSystemRights -eq $_.FileSystemRights -and $accessrule.AccessControlType -eq $_.AccessControlType -and $accessrule.IdentityReference -eq $_.IdentityReference }


Please add space after Where-Object and remove extra space before $accessrule.

iSazonov · 2017-07-10T07:35:51Z

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


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


Indentation.

iSazonov · 2017-07-10T07:36:58Z

test/powershell/Modules/PackageManagement/PackageManagement.Tests.ps1

        $gpp = Get-PackageProvider

-        $gpp | Where-Object{ $_.name -eq "NuGet" } | should not BeNullOrEmpty
+        $gpp | Where-Object{  $_.name -eq "NuGet" } | should not BeNullOrEmpty


Please remove extra space.

iSazonov · 2017-07-10T07:37:12Z

test/powershell/Modules/PackageManagement/PackageManagement.Tests.ps1

+        $gpp | Where-Object{  $_.name -eq "NuGet" } | should not BeNullOrEmpty

-        $gpp | Where-Object{ $_.name -eq "PowerShellGet" } | should not BeNullOrEmpty
+        $gpp | Where-Object{  $_.name -eq "PowerShellGet" } | should not BeNullOrEmpty


Please remove extra space.

iSazonov · 2017-07-10T07:38:08Z

test/tools/OpenCover/OpenCover.psm1

    }
    $CoverageData.PSTypeNames.Insert(0,"OpenCover.CoverageData")
-    Add-Member -InputObject $CoverageData -MemberType ScriptMethod -Name GetClassCoverage -Value { param ( $name ) $this.assembly.classcoverage | Where-Object{$_.classname -match $name } }
+    Add-Member -InputObject $CoverageData -MemberType ScriptMethod -Name GetClassCoverage -Value { param ( $name ) $this.assembly.classcoverage | Where-Object{ $_.classname -match $name } }


Please add space after Where-Object.

lzybkr · 2017-07-10T15:27:37Z

...Modules/Windows-Full/Microsoft.PowerShell.ODataUtils/Microsoft.PowerShell.ODataV4Adapter.ps1

 		$xmlWriter = SaveCDXMLHeader $xmlWriter $Uri $singletonName $singletonName $CmdletAdapter

-        if ($null -eq $associatedEntityType.BaseTyp -and $null -ne $associatedEntityType.BaseTypeStr -and $associatedEntityType.BaseTypeStr -ne '')
+        if ($null -eq $associatedEntityType.BaseType -and $null -ne $associatedEntityType.BaseTypeStr -and $associatedEntityType.BaseTypeStr -ne '')


@anmenaga - this looks like a bug that should get fixed in Windows PowerShell.

@lzybkr : I'm not sure what you mean but the missing 'e' on 'BaseType' was an accidental deletion that happened in the first commit that I corrected

I see - this is a good example of why I'm not a fan of mixing minor changes in one big commit, because I do sometimes review commits by themselves instead of the full PR.

lzybkr · 2017-07-10T15:29:54Z

I'm seeing different sorts of changes in a single commit - and with this many changes, it would be easier to review and safer to keep each refactoring as a single commit.

iSazonov · 2017-07-10T17:11:07Z

+1 for multiple commits. Maybe:

Build.psm1
Rest tool scripts
Test scripts
Rest files.

…econd pull request review

bergmeister · 2017-07-10T18:01:52Z

@iSazonov I have fixed your requested changes about the fine-tuning of the spacing. When I started out doing that, I initially wanted to fix only most PSPossibleIncorrectComparisonWithNull violations but then I came across lots of other things to be improved like e.g. the aliases. Because the contribution guide here recommended to have just one commit per change, I thought I'd try to put it all into one commit for the pull request. I am not sure how one would change the commit history now that the commits are already pushed to the remote. Sorry, maybe I should have rather checked with you before.

iSazonov · 2017-07-10T18:06:57Z

You would rebase and split - see https://stackoverflow.com/questions/6217156/break-a-previous-commit-into-multiple-commits
You already fixed most typos so we can rebase to save maintainer's time.

bergmeister · 2017-07-10T22:05:40Z

@iSazonov OK, thanks. I did not feel confident/adventurous enough to rebase commits that are already pushed to the remote, so I have created a new branch, pulled in my commit changes and made then the 4 separate commits as requested by you. I have created the new PR #4213 for that, so feel free to close this one. Sorry, if this PR turned out to be messier than initially anticipated. I hope you are satisfied with this cleaner version now and I could try to help improve your codebase a bit.

…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'

bergmeister added 2 commits July 9, 2017 21:23

Fixed most PSAvoidUsingCmdletAliases warnings for ForEach-Object (%),…

f26d649

… Where-Object (?), Select-Object and a few others. Fixed some PSUseDeclaredVarsMoreThanAssignment warnings (one must be careful with them as some of those warnings are false positives)

msftclas added the cla-signed label Jul 9, 2017

iSazonov reviewed Jul 10, 2017

View reviewed changes

lzybkr reviewed Jul 10, 2017

View reviewed changes

Fix spacing around Where-Object and indentation as requested in the s…

6001d1a

…econd pull request review

bergmeister mentioned this pull request Jul 10, 2017

Fixes of around 350 PSScriptAnalyzer warnings #4213

Closed

iSazonov closed this Jul 11, 2017

Fix PSScriptAnalyzer warnings #4205

Fix PSScriptAnalyzer warnings #4205

Uh oh!

Conversation

bergmeister commented Jul 9, 2017

Uh oh!

msftclas commented Jul 9, 2017

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergmeister commented Jul 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Jul 10, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzybkr commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jul 10, 2017

Uh oh!

bergmeister commented Jul 10, 2017

Uh oh!

iSazonov commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergmeister commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iSazonov Jul 10, 2017 •

edited

Loading

lzybkr commented Jul 10, 2017 •

edited

Loading

iSazonov commented Jul 10, 2017 •

edited

Loading

bergmeister commented Jul 10, 2017 •

edited

Loading