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 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 Jan 13, 2020
… 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 Jan 13, 2020
@bergmeister bergmeister requested review from JamesWTruher and rjmholt Jan 14, 2020
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>()

This comment has been minimized.

@rjmholt

rjmholt Jan 14, 2020 Member

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>()

This comment has been minimized.

@bergmeister

bergmeister Jan 14, 2020 Author Collaborator

I wanted to differentiate from the _readOnlyAutomaticVariables variable above where PowerShell actually itself throws an error when trying to assign, those variables don't.

This comment has been minimized.

@rjmholt

rjmholt Jan 14, 2020 Member

Well one is readonly variables and the other is just automatic variables, no? So _readonlyVariables and _automaticVariables or _writeableAutomaticVariables maybe?

This comment has been minimized.

@rjmholt

rjmholt Jan 14, 2020 Member

I'd just prefer not to call the variable something that embeds a sentence

This comment has been minimized.

@bergmeister

bergmeister Jan 14, 2020 Author Collaborator

How about _writeableAutomaticVariables and _readonlyAutomaticVariables?

This comment has been minimized.

@rjmholt

rjmholt Jan 14, 2020 Member

👌

bergmeister and others added 2 commits Jan 14, 2020
Co-Authored-By: Robert Holt <rjmholt@gmail.com>
@bergmeister bergmeister merged commit 5d529a3 into PowerShell:master Jan 16, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.