Skip to content
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

Merged
merged 4 commits into from
Jan 16, 2020

Conversation

bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Jan 13, 2020

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

Christoph Bergmeister added 2 commits January 13, 2020 08:31
… that are not read-only but should still not be assigned to in most cases
… into automaticVariables

# Conflicts:
#	Rules/Strings.resx
@bergmeister bergmeister marked this pull request as ready for review January 13, 2020 09:30
Rules/AvoidAssignmentToAutomaticVariable.cs Outdated Show resolved Hide resolved
@@ -33,6 +33,16 @@ public class AvoidAssignmentToAutomaticVariable : IScriptRule
"IsCoreCLR", "IsLinux", "IsMacOS", "IsWindows"
};

private static readonly IList<string> _automaticVariablesThatCouldBeProblematicToAssignTo = new List<string>()
Copy link
Contributor

@rjmholt rjmholt Jan 14, 2020

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)

Suggested change
private static readonly IList<string> _automaticVariablesThatCouldBeProblematicToAssignTo = new List<string>()
private static readonly IList<string> _automaticVariables = new List<string>()

Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Collaborator Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@bergmeister bergmeister merged commit 5d529a3 into PowerShell:master Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants