-
Notifications
You must be signed in to change notification settings - Fork 388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more automatic variables to PSAvoidAssignmentToAutomaticVariables that are not read-only but should still not be assigned to in most cases #1394
Conversation
… that are not read-only but should still not be assigned to in most cases
… into automaticVariables # Conflicts: # Rules/Strings.resx
@@ -33,6 +33,16 @@ public class AvoidAssignmentToAutomaticVariable : IScriptRule | |||
"IsCoreCLR", "IsLinux", "IsMacOS", "IsWindows" | |||
}; | |||
|
|||
private static readonly IList<string> _automaticVariablesThatCouldBeProblematicToAssignTo = new List<string>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we should avoid embedding clausal descriptions in variable names if we can -- and also I can't think of an automatic variable that's not problematic to assign to (since, being automatic, they could be reassigned unexpectedly)
private static readonly IList<string> _automaticVariablesThatCouldBeProblematicToAssignTo = new List<string>() | |
private static readonly IList<string> _automaticVariables = new List<string>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to differentiate from the _readOnlyAutomaticVariables
variable above where PowerShell actually itself throws an error when trying to assign, those variables don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well one is readonly variables and the other is just automatic variables, no? So _readonlyVariables
and _automaticVariables
or _writeableAutomaticVariables
maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just prefer not to call the variable something that embeds a sentence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about _writeableAutomaticVariables
and _readonlyAutomaticVariables
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
Co-Authored-By: Robert Holt <[email protected]>
PR Summary
Closes #712
Closes #1183
The list of variables was extracted from #712 and it i s the union of non-readonly automatic variables in Windows PowerShell and PowerShell Core. Some of those variables can be assigned to in very special cases but the point of this rule is to rather warn that assignment to automatic variables happens and if the author knows what he/she/they are doing then they can/should suppress the rule
PR Checklist
.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
to the beginning of the title and remove the prefix when the PR is ready.