AvoidDefaultValueForMandatoryParameter triggers when the field has specification: Mandatory=value and value!=0 #969
Conversation
…ecification: Mandatory=value and value!=0
|
|
||
| It "returns no violations when the parameter is specified as mandatory=0 and has a default value" { | ||
| $violations = Invoke-ScriptAnalyzer -ScriptDefinition 'Function foo{ Param([Parameter(Mandatory=$false)]$Param1=''val1'', [Parameter(Mandatory)]$Param2=''val2'', $Param3=''val3'') }' | | ||
| Where-Object { $_.RuleName -eq $ruleName } |
There was a problem hiding this comment.
This test seems to be a copy-paste version of the test above if I am not mistaking? I think you meant to change it t have Mandatory = 0. A cleanup of the other unnecessary and confusing parameters $Param2 and $Param3 would be nice as well.
| int mandatoryValue = 0; | ||
| if (namedArgument.ExpressionOmitted | ||
| || (String.Equals(namedArgument.Argument.Extent.Text, "$true", StringComparison.OrdinalIgnoreCase)) | ||
| || (int.TryParse(namedArgument.Argument.Extent.Text, out mandatoryValue) && mandatoryValue != 0)) |
There was a problem hiding this comment.
C# 7 allows you to inline out parameter declaration as (int.TryParse(namedArgument.Argument.Extent.Text, out int mandatoryValue) . This would allow you to delete the int mandatoryValue = 0; line.
But more importantly shouldn't the last comparison be mandatoryValue == 1?
There was a problem hiding this comment.
Currently the powershell interpreter accepts every numeric value of Mandatory parameter. So all the following: Parameter(Mandatory=1), Parameter(Mandatory=-1), Parameter(Mandatory=100) are acceptable and mean that PowerShell treats the parameter as mandatory. Only in case Parameter(Mandatory=0), the parameter is not mandatory.
Hence, I adjusted the rule to the way PowerShell works, but definieily specifying parameter as e.g. Parameter(Mandatory=-100) is not a good coding practice... Maybe we should add some special rule for that?
There was a problem hiding this comment.
Hmm, sounds like we should raise at least an issue in the PowerShell repo to not allow values other than 0/1. If someone specified something else then it is most likely a so called fat-finger error. I can see the point of warning in PSSA but I am not sure if it is worth the effort and computational expense of having an extra rule just for that specifically but maybe rather a more generic rule for syntax validation ala PossibleUninentionalSyntax that we could use in the future for warnings unrelated to a rule itself. Therefore this question/issue should not concern/block this PR, which is a set of complete changes leading to improved behaviour of PSSA, therefore I will approve.
PR Summary
AvoidDefaultValueForMandatoryParameter triggers when the field has specification: Mandatory=value and value is not equal to 0.
Fixes: #877
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets. Please mark anything not applicable to this PRNA.WIP:to the beginning of the title and remove the prefix when the PR is ready