[Pending] Finally we add the code linting and its tests!

[chawyehsu]

What does this big pull-request do: we use PSScriptAnalyzer for code linting.

1. Improve all current *.ps1 files to pass PSScriptAnalyzer check. notes: some rules have been excluded for special reason, you can take a look at PSScriptAnalyzerSettings.psd1.
2. Add PSScriptAnalyzer integration for vscode.
3. Improve tests to ignore Pester generated TestResults.xml.
4. Add PSScriptAnalyzer checking into tests, CI included.

There are some opinionated changes, so this pull-request is pending and looking for reviews.

[rasa]

What’s the reason it puts $null first in comparisons ($null -eq $var)?

[chawyehsu]

@rasa PSScriptAnalyzer RuleDocumentation: PSPossibleIncorrectComparisonWithNull

[rrelmy]

lgtm 👍

[stkb]

I have issues with a couple of these. The rest are ok I guess, although personally I'd allow % and ? for Foreach-Object and Where-Object since they're so well known.

PSUseBOMForUnicodeEncodedFile

Is there a specific reason for this? BOMs are "neither required not recommended [for utf8]" by the Unicode standard (section 2.6) and create more problems than they solve in my experience, expecially with cross-platform tools. PowerShell will also be removing the BOM from the default UTF-8 encoding.

If we could instead ensure that say, all files were utf8 encoded (without BOM), that would be good imo.

PSPossibleIncorrectComparisonWithNull

I don't see the usefulness of this. The first reason given is that it can produce an unexpected result when comparing $null to something that might be an array? But If you're comparing something that might be an array to $null you're most likely doing something wrong anyway. And secondly that's not limited to nulls; you'll get unexpected behavior whenever you do $maybeArray -eq anything if you don't know how -eq works there.

The second reason given: PowerShell will perform type casting left to right, resulting in incorrect comparisons when $null is cast to other scalar types.
I don't really understand this; is there an example?

[r15ch13]

Cool! 😎

This variant shows which file violates a rule:

$repo_dir = (Get-Item $MyInvocation.MyCommand.Path).directory.parent.FullName
$scoop_modules = Get-ChildItem $repo_dir -Filter "*.ps1" -Recurse | Select-Object -ExpandProperty Directory -Unique
$linting_settings = Get-Item -Path "$repo_dir\PSScriptAnalyzerSettings.psd1"

Describe "Linting all modules" {
    $rules = Get-ScriptAnalyzerRule
    foreach($directory in $scoop_modules) {
        Context "Linting $directory\*.ps1" {
            foreach($rule in $rules) {
                It "Should not return any violation for the rule: $($rule.RuleName)" {
                    $result = Invoke-ScriptAnalyzer -Path $directory.FullName -Settings $linting_settings.FullName -IncludeRule $rule.RuleName
                    if($result) {
                        Write-Warning ($result | Out-String)
                    }
                    $result.Count | Should Be 0
                }
            }
        }
    }
}

Two interesting articles:

• https://mathieubuisson.github.io/psscriptanalyzer-appveyor/
• https://mathieubuisson.github.io/psscriptanalyzer-first-class-citizen/

[chawyehsu]

@stkb

1. For aliases like % and ?, codes will be more readable by using Foreach-Object and Where-Object, besides, @bergmeister talked about it in pull/2075#issuecomment-370164069:

PSAvoidUsingCmdletAliases. It is good that you removed them, the PowerShell team is planning to take out aliases in one of the next versions (but one will still be able to use them by installing an optional module) because they make so many problems (mainly due to clashes with other binaries in the Windows and Linux world).

2. Your're right, no BOM is better. Combine this commit and this commit, we just remove that non-ASCII character —— and keepbin/install.ps1 without BOM. Before this pull-request, I don't know that there is a non-ASCII character in bin/install.ps1, which cause fail the check.
   (doc: RuleDocumentation/UseBOMForUnicodeEncodedFile.md). Maybe it's just a typo.
3. It's about array, yes, a discussion about it can be found at PSScriptAnalyzer/issues#477. It doesn't matter if we do know the left-hand value wouldn't be unexpectedly an array, so it calls PossibleIncorrectComparisonWithNull, and if the 'noes' have it, it okay to exclude this rule, here I just follow the rule for PSScriptAnalyzer's 'best practices'.

[chawyehsu]

@r15ch13 Current implementation does show which file violates a rule if it violates a rule. e.g.

Context Linting scoop-which.ps1
      [-] Passes PSScriptAnalyzer 68ms
        Expected 0, but got 1.
        9:                   (Invoke-ScriptAnalyzer $module.FullName -Settings $linting_settings.FullName).count | Should Be 0
        at Invoke-LegacyAssertion, C:\Users\<omit>\Documents\WindowsPowerShell\Modules\Pester\4.3.1\Functions\Assertions\Should.ps1: line 188
        at <ScriptBlock>, C:\Users\<omit>\workspace\scoop\test\Scoop-Linting.Tests.ps1: line 9

[bergmeister]

You might want to consider using a "*.ps*1 filter to also include .psm1 and .psd1 files (there are even specific rules just for manifests)

[chawyehsu]

Thank you for pointing it out that.

[bergmeister]

I'm glad you find the tool useful. Here are some remarks:

• The way how I integrate PSSA into CI is to have an assertion for each rule, which gives you better messages when the tests fail. An old example that I quickly grabbed is here although by looking at it, I would move the last foreach into the It block to avoid having too many test results
• PSUseBOMForUnicodeEncodedFile: This rule is questionable and maybe out of date, see here. From personal experience: This rule once helped me but this was due to a bug of an Alpha version of PowerShell Core on non-Windows systems. I think it is OK to ignore this rule for now. Even the PowerShell team has settled on UTF-8 without BOM for better interoperability on Linux.
• PSPossibleIncorrectComparisonWithNull: the documentation could probably do a better job and give more examples. At the end it boils down to how comparison operators work ( get-help about_comparison), which is not intuitive and has many gotchas. I highly recommend using this rule in order to be on the safe side. You can otherwise end up with scenarios where e.g. if ($a -ne $null){} always evaluates to true. When you fix your violations, make sure your code was not accidentally relying on getting into the wrong code path. Take this example, where the code always goes into the else clause because the comparison operators do not return anything at all, which makes the if statement fall through.

$a=@()
if ($a -eq $null){"True"}else{"false"}
if ($a -ne $null){"True"}else{"false"}

We expect a new release in the next weeks with a ton of improvements and features. With that come also 2 new rules about assignment to automatic variables and possible incorrect/unintentional usage of assignment operators.

[r15ch13]

@h404bi yes it shows the file, but it doesn't show which violation at which line. I think it's important to just take a look at the CI result and know what has to be fixed.
Also calling Invoke-ScriptAnalyzer with directory\*.ps1 is faster than calling it for each file.

Another approach based on @bergmeister's example file

$repo_dir = (Get-Item $MyInvocation.MyCommand.Path).directory.parent.FullName
$scoop_modules = Get-ChildItem $repo_dir -Filter "*.ps*1" -Recurse | Select-Object -ExpandProperty Directory -Unique
$linting_settings = Get-Item -Path "$repo_dir\PSScriptAnalyzerSettings.psd1"
$rules = Get-ScriptAnalyzerRule

Describe "Linting all modules" {
    foreach($directory in $scoop_modules) {
        Context "Linting $directory\*.ps1" {
            $analysis = Invoke-ScriptAnalyzer -Path $directory.FullName -Settings $linting_settings.FullName
            foreach($rule in $rules) {
                It "Should pass: $rule" {
                    If ($analysis.RuleName -contains $rule) {
                        $analysis | Where-Object RuleName -eq $rule -outvariable failures
                        Write-Warning ($failures | Out-String)
                        $failures.Count | Should Be 0
                    }
                }
            }
        }
    }
}

[chawyehsu]

@r15ch13 minor tweaks as follow:

$repo_dir = (Get-Item $MyInvocation.MyCommand.Path).directory.parent.FullName
$scoop_modules = Get-ChildItem $repo_dir -Filter "*.ps*1" -Recurse | Select-Object -ExpandProperty Directory -Unique
$linting_settings = Get-Item -Path "$repo_dir\PSScriptAnalyzerSettings.psd1"
$rules = Get-ScriptAnalyzerRule

Describe "Linting all *.ps*1 modules" {
    foreach($directory in $scoop_modules) {
        Context "Linting *.ps*1 modules in '$directory'" {
            $analysis = Invoke-ScriptAnalyzer -Path $directory.FullName -Settings $linting_settings.FullName
            foreach($rule in $rules) {
                It "Should pass: $rule" {
                    If ($analysis.RuleName -contains $rule) {
                        $analysis | Where-Object RuleName -eq $rule -outvariable failures
                        Write-Warning ($failures | Out-String)
                        $failures.Count | Should Be 0
                    }
                }
            }
        }
    }
}

But I want to point it out that is it necessary to log out all rules into testresults? It looks like a spam, what I think is that if it passes, no mess logs, if failed, output the error stack. 😂 Would commit this tweak if approved.

[r15ch13]

@h404bi Tests should show detailed information and AppVeyor doesn't care about spam 😁

But I worked a bit more on it. Now with less spam and better output (showing clickable file path with line number)

$repo_dir = (Get-Item $MyInvocation.MyCommand.Path).directory.parent.FullName

Describe "PSScriptAnalyzer" {
    $scoop_modules = Get-ChildItem $repo_dir -Recurse -Include *.psd1, *.psm1, *.ps1
    $scoop_modules = $scoop_modules | Where-Object { $_.DirectoryName -notlike '*\supporting*' -and $_.DirectoryName -notlike '*\test*' }
    $scoop_modules = $scoop_modules | Select-Object -ExpandProperty Directory -Unique

    Context "Checking ScriptAnalyzer" {
        It "Invoke-ScriptAnalyzer Cmdlet should exist" {
            { Get-Command Invoke-ScriptAnalyzer -ErrorAction Stop } | Should Not Throw
        }
        It "PSScriptAnalyzerSettings.ps1 should exist" {
            Test-Path "$repo_dir\PSScriptAnalyzerSettings.psd1" | Should Be $true
        }
        It "There should be files to test" {
            $scoop_modules.Count | Should Not Be 0
        }
    }

    $linting_settings = Get-Item -Path "$repo_dir\PSScriptAnalyzerSettings.psd1"

    Context "Linting all *.psd1, *.psm1 and *.ps1 files" {
        foreach($directory in $scoop_modules) {
            $analysis = Invoke-ScriptAnalyzer -Path $directory.FullName -Settings $linting_settings.FullName
            It "Should pass: $directory" {
                $analysis.Count | Should Be 0
            }
            if($analysis) {
                foreach($result in $analysis) {
                    switch -wildCard ($result.ScriptName) {
                        '*.psm1' { $type = 'Module' }
                        '*.ps1'  { $type = 'Script' }
                        '*.psd1' { $type = 'Manifest' }
                    }
                    Write-Host -f Yellow "      [*] $($result.Severity): $($result.Message)"
                    Write-Host -f Yellow "          $($result.RuleName) in $type`: $directory\$($result.ScriptName):$($result.Line)"
                }
            }
        }
    }
}

[chawyehsu]

@r15ch13 Cool, that looks nice, I updated the test with your variant.