Add warnings about read-only automatic variables introduced in PowerShell 6.0 #917
Conversation
|
Good idea, certainly a nice-to-have, but I've implemented it now. |
|
|
||
| [System.Array] $warnings = Invoke-ScriptAnalyzer -ScriptDefinition "function foo(`$$VariableName){}" | Where-Object {$_.RuleName -eq $ruleName } | ||
| $warnings.Count | Should -Be 1 | ||
| $warnings.Severity | Should -Be $readOnlyVariableSeverity | ||
| $warnings.Severity | Should -Be $ExpectedSeverity |
There was a problem hiding this comment.
I see the above pattern a lot, but it worries me. I would think that the Where-Object clause would obfuscate additional unexpected errors. Shouldn't we know the total amount of errors we expect and not have to filter for only what we expect?
There was a problem hiding this comment.
There is no 'right' answer, I think. There is the approach of having either isolated unit tests (with the benefit of lower maintenance) or doing integration testing as part of the test. We are still running all rules against it, so we would probably still get a test failure if one rule unexpectedly threw an exception but we would not have to adapt tests too much if rules changed. I will leave this decision up to you and will therefore remove the Where-Object and replace it with $warnings.RuleName | Should -Be $ruleName'. In one test case I will use -ExcludeRule PSUseDeclaredVarsMoreThanAssignments`
|
@bergmeister we've got merge conflicts at this point - could you resolve and push? |
PR Summary
Related issue (but not sufficient to close it): #858
The variables
IsCoreCLR,IsLinux,IsMacOS,IsWindowswere introduced only in PowerShell 6.0 as read-only, automatic variables and attempting to assign to them will result in an error being thrown at runtime.Therefore warn against using it but reduce severity from
ErrortoWarningfor those variables since they do not affect Windows PowerShell, an appropriate message has been added with this additional informationPR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets. Please mark anything not applicable to this PRNA.WIP:to the beginning of the title and remove the prefix when the PR is ready