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

AvoidUsingPositionalParameters does not seem to be triggered #863

Closed
LaurentDardenne opened this issue Feb 2, 2018 · 5 comments · Fixed by #867
Closed

AvoidUsingPositionalParameters does not seem to be triggered #863

LaurentDardenne opened this issue Feb 2, 2018 · 5 comments · Fixed by #867

Comments

@LaurentDardenne
Copy link

cd c:\temp
Invoke-ScriptAnalyzer -ScriptDefinition "Write-host 'Get-ChildItem  *.txt'"
#ok one error

Invoke-ScriptAnalyzer -ScriptDefinition "Get-ChildItem  *.txt" -verbose
#nothing

VERBOSE: Cannot find a settings file.
VERBOSE: Analyzing Script Definition.
VERBOSE: Running PSAvoidUsingCmdletAliases rule.
VERBOSE: Running PSAvoidDefaultValueSwitchParameter rule.
VERBOSE: Running PSAvoidUsingEmptyCatchBlock rule.
VERBOSE: Running PSAvoidGlobalAliases rule.
VERBOSE: Running PSAvoidGlobalFunctions rule.
VERBOSE: Running PSAvoidDefaultValueForMandatoryParameter rule.
VERBOSE: Running PSAvoidGlobalVars rule.
VERBOSE: Running PSAvoidInvokingEmptyMembers rule.
VERBOSE: Running PSAvoidNullOrEmptyHelpMessageAttribute rule.
VERBOSE: Running PSReservedCmdletChar rule.
VERBOSE: Running PSReservedParams rule.
VERBOSE: Running PSAvoidShouldContinueWithoutForce rule.
VERBOSE: Running PSAvoidUsingUserNameAndPassWordParams rule.
VERBOSE: Running PSAvoidUsingComputerNameHardcoded rule.
VERBOSE: Running PSAvoidUsingConvertToSecureStringWithPlainText rule.
VERBOSE: Running PSAvoidUsingDeprecatedManifestFields rule.
VERBOSE: Running PSAvoidUsingInvokeExpression rule.
VERBOSE: Running PSAvoidUsingPlainTextForPassword rule.
VERBOSE: Running PSAvoidUsingWMICmdlet rule.
VERBOSE: Running PSAvoidUsingWriteHost rule.
VERBOSE: Running PSMisleadingBacktick rule.
VERBOSE: Running PSMissingModuleManifestField rule.
VERBOSE: Running PSPossibleIncorrectComparisonWithNull rule.
VERBOSE: Running PSProvideCommentHelp rule.

   VERBOSE: Running PSAvoidUsingPositionalParameters rule.

VERBOSE: Running PSUseBOMForUnicodeEncodedFile rule.
VERBOSE: Running PSUseApprovedVerbs rule.
VERBOSE: Running PSUseCompatibleCmdlets rule.
VERBOSE: Running PSUseDeclaredVarsMoreThanAssignments rule.
VERBOSE: Running PSUseLiteralInitializerForHashtable rule.
VERBOSE: Running PSUseOutputTypeCorrectly rule.
VERBOSE: Running PSUsePSCredentialType rule.
VERBOSE: Running PSUseCmdletCorrectly rule.
VERBOSE: Running PSUseShouldProcessForStateChangingFunctions rule.
VERBOSE: Running PSUseSingularNouns rule.
VERBOSE: Running PSUseSupportsShouldProcess rule.
VERBOSE: Running PSUseToExportFieldsInManifest rule.
VERBOSE: Running PSUseUTF8EncodingForHelpFile rule.
VERBOSE: Running PSShouldProcess rule.


#Module PSScriptAnalyzer version 1.16.1    
$PSVersionTable

Name                           Value
----                           -----
PSVersion                      5.1.14409.1012
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0, 5.0, 5.1.14409.1012}
BuildVersion                   10.0.14409.1012
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

#identical with PS core 6.0.1

Severity must be 'Information' or 'Warning' ?

@bergmeister
Copy link
Collaborator

bergmeister commented Feb 2, 2018

You are right that the documentation does not match the implementation. I think warning would make more sense because not using positional parameters may cause a problem since the definition of warning level is:

This warning may cause a problem or does not follow PowerShell's recommended guidelines.

You are also right that the 2nd example seems to be embarrassing because it is very similar to the example given in the rule documentation here, which does not work either (and I think the given example is not good either because the command in the string is confusing and has nothing to do with the rule/warning). But I think your first example suffers from the same issue as well because it does not produce AvoidUsingPositionalParameters either (I think you were mislead because it produced PSAvoidUsingWriteHost correctly)

I checked the existing test here, which uses Get-Command "abc" 4 4.3 as an example for the positive test, which works. By trying more arguments with Write-Host, it seems that the rule only gets triggered when there are 3 or more parameters. I then looked at the negative tests here and this comment with the examples below gives an indication that it might be by design that the rule will not trigger with less than 3 arguments in some cases. By looking at the commit here it becomes clear from the commit message Change positional parameter rule so it will only be triggered if we have 3 positional parameters that this was by design at the time being and there is even a referenced issue stating that: #301

So much about the current state but we can of course work together to improve the design. For sure, the documentation needs to be updated in the meantime.

@LaurentDardenne
Copy link
Author

Thanks for the links.
I looked in the file change.log but I did not find any indication on this change.

So much about the current state but we can of course work together to improve the design.

For me it's more a problem of specification, what is the rule to respect?
In PowerShellPracticeAndStyle (see Use full parameter names.) The rule is simple and clear.
I have always believed, and I do not think I am the only one, that the implementation of this rule corresponds to its statement.

I am of the opinion of Keith, this would allow everyone to apply the rule according to his preferences.
A parametric rule with a trigger threshold set to 3 by default may be a possible solution.

@bergmeister
Copy link
Collaborator

Ok. So basically what you want in a nutshell is the ability to override the default configuration of the threshold number? This could maybe be done via a new setting in the settings file.

@LaurentDardenne
Copy link
Author

LaurentDardenne commented Feb 4, 2018

So basically what you want in a nutshell is the ability to override the default configuration of the threshold number?

Yes.

@bergmeister
Copy link
Collaborator

@LaurentDardenne Feel free to create a new, separate issue for your feature request of customizing the rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants