-
Notifications
You must be signed in to change notification settings - Fork 382
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 ErrorView to SpecialVars.cs #1865
Add ErrorView to SpecialVars.cs #1865
Conversation
8d953ae
to
b0f0486
Compare
@microsoft-github-policy-service agree |
@@ -114,6 +116,7 @@ static SpecialVars() | |||
/* ConfirmPreference */ typeof(ConfirmImpact), | |||
/* ProgressPreference */ typeof(Enum), | |||
/* InformationPreference */ typeof(ActionPreference), | |||
/* ErrorView */ typeof(Enum), //ErrorView type not available on PS3 |
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.
There is PSV3
variable defined as we do conditional compilation for each version of PS (3, 4, 5 and 'Core'). You could use this to use Enum only for the v3 compiled version but the correct types for others.
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.
Okay I can do that, however I'm still not sure why this type array is here since nobody seems to reference it, should it be expanded despite this?
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.
@bergmeister @JamesWTruher what is the point of this enum? Can we just delete the whole thing, and merge this PR already?
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.
@Jaykul Whilst it's not directly referenced, it is still needed as it clearly solves the issue of not flagging false positive for ErrorView. I presume the rule is using reflection. The PSV3
variable is a pragma, i.e. a build time variable as we use different PowerShell version SDKs when we build against the different versions.
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.
Honestly, maybe we should probably just type this as string.
The ErrorView
enum is new in PS7 and it's not enforced. The variable can be overwritten, and now that it's an Enum in PS7, it has to be overwritten by anyone who wants to improve the error view (such as my ErrorView module, which actually works on old versions of PowerShell)...
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.
Overall looks good, thanks. Just one idea around the type problem you mentioned
PR Summary
Fixes #1749
No warning is issued for
Errorview
by rulePSUseDeclaredVarsMoreThanAssignment
ErrorView
toSpecialVars.cs
Additional Notes
I could not use the correct type for
ErrorView
as the type ErrorView is not available in PS3 and instead declared it's type as anEnum
.However I found that inside
SpecialVars.cs
the array of types :Type[] PreferenceVariableTypes
, is not used anywhere, so is there a reason for it to exist?PSScriptAnalyzer/Engine/SpecialVars.cs
Line 107 in 77f6500
PR Context
When we try to assign a new state to a Preference Variable like
ErrorView
it should not be flagged as a warning by the rulePSUseDeclaredVarsMoreThanAssignment
. In order to prevent the rule from showing this warning there is a check for built in variables:PSScriptAnalyzer/Rules/UseDeclaredVarsMoreThanAssignments.cs
Line 203 in 77f6500
This checks for the presence of the Preference Variable's declaration inside the
SpecialVars.cs
file. If it's in that filePSUseDeclaredVarsMoreThanAssignment
stops throwing the warning.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.