Enable further analyzer rules for better code styling control and more (OSOE-501)

[BenedekFarkas]

Review https://learn.microsoft.com/en-us/powershell/utility-modules/psscriptanalyzer/rules/readme and out of the ones that aren't enabled by default, select the appropriate ones. Repositories to check: OSOCE, Infrastructure Scripts, Utility Scripts.

Complete list:

1. ✅PSAlignAssignmentStatement: The authors recommend this for readability (I like it better too) and it's easy to fix and maintain in VS Code with just one setting. Moved to Enable PSAlignAssignmentStatement analyzer rule (OSOE-538) #21.
2. ✅PSAvoidLongLines: For C#, we have guidelines (vertical ruler) for 120 and 140 characters and have a hard limit at 150, so 150 here would be nice. We have 72 violations overall at the 150 limit. Moved to Enable PSAvoidLongLines analyzer rule (OSOE-540) #23.
3. ✔️PSAvoidSemicolonsAsLineTerminators: This is super simple but requires update to 1.21.0 (doesn't seem to be any breaking changes so far).
4. ✔️PSAvoidUsingDoubleQuotesForConstantString: Consistency! VS Code could auto-fix this if powerShell.codeFormatting.useConstantStrings is enabled. We're currently only surfacing Warnings and Errors (but not Information entries). This rule has Information severity, but the docs is outdated on that, so I created an issue and PR: Documentation is outdated on the AvoidUsingDoubleQuotesForConstantString rule's Severity and Configurability MicrosoftDocs/PowerShell-Docs-Modules#128
   We can lower the threshold (I'd actually rather move it to the settings file from Invoke-Analyzers.ps1) to Information and then disable the rules that we don't care about but enabled by default (there are only 2 such possible rules).
5. ✔️PSPlaceCloseBrace
6. ✔️PSPlaceOpenBrace
7. ⛔PSUseCompatibleCommands: Skip (at least I don't see its usefulness at the moment).
8. ⛔PSUseCompatibleSyntax: I don't think we need this, because we run analysis on both the latest Windows PowerShell and PowerShell Core, so incompatible syntax is already surfaced by runtime errors.
9. ⛔PSUseCompatibleTypes: Probably not interesting now.
10. ✔️PSUseConsistentIndentation: Definitely useful!
11. ✔️UseConsistentWhitespace: Definitely useful!
12. ✔️PSUseCorrectCasing: Also great for consistency. Not yet enabled because of Enabling PSUseCorrectCasing causes error while processing code that doesn't violate it PowerShell/PSScriptAnalyzer#1881. Moved to (blocked) Enable PSUseCorrectCasing analyzer rule (OSOE-539) #22.

Jira issue

[BenedekFarkas]

When PSUseConsistentWhitespace.CheckParameter is $true, this code from Wait-SqlServer generates two warnings:

if ($Env:RUNNER_OS -eq "Windows")
{
    sqlcmd -b -S .\SQLEXPRESS -Q "SELECT @@SERVERNAME as ServerName" 2>&1>$null
}
else
{
    sqlcmd -b -U sa -P 'Password1!' -Q "SELECT @@SERVERNAME as ServerName" 2>&1>$null
}

VSCode autoformatting changes this to the following, making the warnings go away, but I don't think this is correct:

if ($Env:RUNNER_OS -eq "Windows")
{
    sqlcmd -b -S .\SQLEXPRESS -Q "SELECT @@SERVERNAME as ServerName" >$null
}
else
{
    sqlcmd -b -U sa -P 'Password1!' -Q "SELECT @@SERVERNAME as ServerName" >$null
}

@DAud-IcI do you know if we can just update this to save into a variable or something similar to instead of the "exotic" syntax?

[BenedekFarkas]

Also, when PSUseCorrectCasing is enabled, the error below is thrown locally because of this exact line:

Update-VisualStudioSolutionNuGetPackages -Path (Get-Location).Path -ProjectNameFilter "Lombiq.*" -PackageNameFilter "Lombiq.*"

Invoke-ScriptAnalyzer: [...]\tools\Lombiq.Analyzers.PowerShell\Lombiq.Analyzers.PowerShell\Invoke-Analyzer.ps1:117
Line |
 117 |  … ch-Object { Invoke-ScriptAnalyzer -Path $PSItem.FullName @analyzerPar …
     |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Cannot find an overload for ".ctor" and the argument count: "1".

Any ideas? GHA execution is fine though.

[sarahelsaig]

do you know if we can just update this to save into a variable or something similar to instead of the "exotic" syntax?

mfw someone calls the stream redirection syntax that has been common in pretty much all shells since the dawn of time "exotic".

2>&1 tells the shell to merge the standard error stream into the standard output stream on-the-fly. That's where sqlcmd outputs all the warnings. If for some reason we want to show SQL client warnings in the logs, then the reformatted version is ok. Otherwise for this scenario we could use 2>$null >$null which is a more verbose equivalent. There you individually send each stream into NUL instead of merging the two first.

---

Also, when PSUseCorrectCasing is enabled, the error below is thrown locally

It appears the script throws a PowerShell syntax error, it can't even be caught using try-catch.

The reason it "works" in GHA is because it doesn't produce the error GHA expects.

(here contrast the raw error message with the neat GHA style errors caused by the sloppily formatted try-catch I added)

So I'm afraid the PSUseCorrectCasing is just broken? I'd avoid using it for the time being. Can you open an issue about this at PowerShell/PSScriptAnalyzer? I've isolated a minimal repro that can be used with just their module and vanilla pwsh:

'@{ Rules = @{ PSUseCorrectCasing = @{ Enable = $true } } }' > rules.psd1
Invoke-ScriptAnalyzer -Settings rules.psd1 -ScriptDefinition 'Update-VisualStudioSolutionNuGetPackages'

[BenedekFarkas]

mfw someone calls the stream redirection syntax that has been common in pretty much all shells since the dawn of time "exotic".

Ha! :D
Well, I never had to use it, so I'm considering myself lucky, then!

---

If for some reason we want to show SQL client warnings in the logs, then the reformatted version is ok.

So, the reformatted version doesn't break it and $? still captures the status of the command execution, right? Then we can just put | Out-Null after it, so there won't be any visible output.

---

So I'm afraid the PSUseCorrectCasing is just broken? I'd avoid using it for the time being. Can you open an issue about this at PowerShell/PSScriptAnalyzer? I've isolated a minimal repro that can be used with just their module and vanilla pwsh:

Thanks! The weird thing is that the original code doesn't violate PSUseCorrectCasing either and that analyzer works just fine otherwise.
See: PowerShell/PSScriptAnalyzer#1881

[BenedekFarkas]

I don't know what the heck is going on with the Update-VisualStudioSolutionNuGetPackages string in PowerShell, but auto-formatting on in the Update-LombiqNuGetPackages.ps1 doesn't work either (e.g., for fixing PSAvoidUsingDoubleQuotesForConstantString violations).
If I remove a single character from the Update-VisualStudioSolutionNuGetPackages cmdlet name (doesn't matter which one), then auto-formatting works again, and the error exhibited above also goes away...

[0liver]

The second part of the commandlet name is currently 33 characters long - maybe 32 is a magical number here?! The name is quite long, admittedly.

Why does VisualStudio have to be part of it, anyway?

[BenedekFarkas]

I noticed that too, but we have cmdlets with longer names, e.g., Remove-AzureWebAppSqlDatabaseContainedUser.
Update-VisualStudioSolutionNuGetPackages and one of the underlying cmdlets are specifically reading information from VS solutions (they are a wrapper for the dotnet sln command).

[BenedekFarkas]

I factored out 3 analyzers to separate issue (see them marked with ✅ in the description), so this is done.

[0liver]

A very wise decision, let me tell you!

[BenedekFarkas]

Check this out! PowerShell/PSScriptAnalyzer#1881 (comment)

PSUseCorrectCasing is now also enabled.