Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd warnings about read-only automatic variables introduced in PowerShell 6.0 #917
Conversation
|
I wonder if this should be an error on Core and a warning on non-Core? |
|
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 |
This comment has been minimized.
This comment has been minimized.
JamesWTruher
Mar 20, 2018
Member
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?
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 20, 2018
•
Author
Collaborator
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? |
bergmeister commentedMar 3, 2018
•
edited
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