Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAvoidDefaultValueForMandatoryParameter triggers when the field has specification: Mandatory=value and value!=0 #969
Conversation
…ecification: Mandatory=value and value!=0
|
Thank you. Looks mostly OK but I have one question about one comparison, the rest are just minor comments. |
|
|
||
| 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 } |
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 13, 2018
•
Collaborator
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.
This comment has been minimized.
This comment has been minimized.
| 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)) |
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 13, 2018
Collaborator
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?
This comment has been minimized.
This comment has been minimized.
kalgiz
Apr 16, 2018
Author
Contributor
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?
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 16, 2018
Collaborator
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.
kalgiz commentedApr 12, 2018
•
edited
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