From 08765c2f0f36fa8eaf907ac2470ae64a9cfc18a0 Mon Sep 17 00:00:00 2001 From: Quoc Truong Date: Mon, 14 Sep 2015 09:47:27 -0700 Subject: [PATCH 1/2] Change positional parameter rule so it will only be triggered if we have 3 positional parameters --- Engine/Helper.cs | 8 +++++++- Rules/AvoidPositionalParameters.cs | 8 ++++---- Tests/Rules/AvoidPositionalParameters.ps1 | 7 ++----- Tests/Rules/AvoidPositionalParameters.tests.ps1 | 6 +++--- Tests/Rules/AvoidPositionalParametersNoViolations.ps1 | 9 ++++++++- 5 files changed, 24 insertions(+), 14 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 3a09d2c56..7dccf0841 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -283,8 +283,9 @@ public bool HasSplattedVariable(CommandAst cmdAst) /// Given a commandast, checks whether positional parameters are used or not. /// /// + /// only return true if more than three positional parameters are used /// - public bool PositionalParameterUsed(CommandAst cmdAst) + public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanThreePositional = false) { if (cmdAst == null || cmdAst.GetCommandName() == null) { @@ -351,6 +352,11 @@ public bool PositionalParameterUsed(CommandAst cmdAst) arguments += 1; } + if (moreThanThreePositional && arguments < 3) + { + return false; + } + return arguments > parameters; } diff --git a/Rules/AvoidPositionalParameters.cs b/Rules/AvoidPositionalParameters.cs index a52ac550b..9744096bf 100644 --- a/Rules/AvoidPositionalParameters.cs +++ b/Rules/AvoidPositionalParameters.cs @@ -45,7 +45,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (cmdAst.GetCommandName() == null) continue; if (Helper.Instance.GetCommandInfo(cmdAst.GetCommandName()) != null - && Helper.Instance.PositionalParameterUsed(cmdAst)) + && Helper.Instance.PositionalParameterUsed(cmdAst, true)) { PipelineAst parent = cmdAst.Parent as PipelineAst; @@ -55,14 +55,14 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (parent.PipelineElements[0] == cmdAst) { yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, cmdAst.GetCommandName()), - cmdAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName, cmdAst.GetCommandName()); + cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, cmdAst.GetCommandName()); } } // not in pipeline so just raise it normally else { yield return new DiagnosticRecord(string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingPositionalParametersError, cmdAst.GetCommandName()), - cmdAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName, cmdAst.GetCommandName()); + cmdAst.Extent, GetName(), DiagnosticSeverity.Information, fileName, cmdAst.GetCommandName()); } } } @@ -109,7 +109,7 @@ public SourceType GetSourceType() /// public RuleSeverity GetSeverity() { - return RuleSeverity.Warning; + return RuleSeverity.Information; } /// diff --git a/Tests/Rules/AvoidPositionalParameters.ps1 b/Tests/Rules/AvoidPositionalParameters.ps1 index f2cd77ede..93e3acc38 100644 --- a/Tests/Rules/AvoidPositionalParameters.ps1 +++ b/Tests/Rules/AvoidPositionalParameters.ps1 @@ -1,5 +1,2 @@ -Get-Content Test -Get-ChildItem Tests -Write-Output "I don't want to use positional parameters" -Split-Path "RandomPath" -Leaf -Get-Process | ForEach-Object {Write-Host $_.name -foregroundcolor cyan} \ No newline at end of file +# give it 3 positional parameters +Get-Command "abc" 4 4.3 \ No newline at end of file diff --git a/Tests/Rules/AvoidPositionalParameters.tests.ps1 b/Tests/Rules/AvoidPositionalParameters.tests.ps1 index 2e5156edd..fb7f151ea 100644 --- a/Tests/Rules/AvoidPositionalParameters.tests.ps1 +++ b/Tests/Rules/AvoidPositionalParameters.tests.ps1 @@ -1,5 +1,5 @@ Import-Module PSScriptAnalyzer -$violationMessage = "Cmdlet 'Write-Host' has positional parameter. Please use named parameters instead of positional parameters when calling a command." +$violationMessage = "Cmdlet 'Get-Command' has positional parameter. Please use named parameters instead of positional parameters when calling a command." $violationName = "PSAvoidUsingPositionalParameters" $directory = Split-Path -Parent $MyInvocation.MyCommand.Path $violations = Invoke-ScriptAnalyzer $directory\AvoidPositionalParameters.ps1 | Where-Object {$_.RuleName -eq $violationName} @@ -8,8 +8,8 @@ $noViolationsDSC = Invoke-ScriptAnalyzer -ErrorAction SilentlyContinue $director Describe "AvoidPositionalParameters" { Context "When there are violations" { - It "has 4 avoid positional parameters violation" { - $violations.Count | Should Be 5 + It "has 1 avoid positional parameters violation" { + $violations.Count | Should Be 1 } It "has the correct description message" { diff --git a/Tests/Rules/AvoidPositionalParametersNoViolations.ps1 b/Tests/Rules/AvoidPositionalParametersNoViolations.ps1 index 874ef0bd7..1c3841846 100644 --- a/Tests/Rules/AvoidPositionalParametersNoViolations.ps1 +++ b/Tests/Rules/AvoidPositionalParametersNoViolations.ps1 @@ -13,4 +13,11 @@ get-service-computername localhost | where {($_.status -eq "Running") -and ($_.C function TestExternalApplication { & "c:\Windows\System32\Calc.exe" parameter1 -} \ No newline at end of file +} + +# less than 3 arguments so rule won't trigger +Get-Content Test +Get-ChildItem Tests +Write-Output "I don't want to use positional parameters" +Split-Path "RandomPath" -Leaf +Get-Process | ForEach-Object {Write-Host $_.name -foregroundcolor cyan} \ No newline at end of file From 85c8b9d84bb946c50bbb0034b257088245c04b93 Mon Sep 17 00:00:00 2001 From: Quoc Truong Date: Mon, 14 Sep 2015 10:59:23 -0700 Subject: [PATCH 2/2] Fix test failures --- Tests/Engine/GetScriptAnalyzerRule.tests.ps1 | 2 +- Tests/Engine/InvokeScriptAnalyzer.tests.ps1 | 8 ++++---- Tests/Engine/RuleSuppression.tests.ps1 | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 index 2606aab63..63895955c 100644 --- a/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 +++ b/Tests/Engine/GetScriptAnalyzerRule.tests.ps1 @@ -118,7 +118,7 @@ Describe "TestSeverity" { It "filters rules based on multiple severity inputs"{ $rules = Get-ScriptAnalyzerRule -Severity Error,Information - $rules.Count | Should be 13 + $rules.Count | Should be 14 } It "takes lower case inputs" { diff --git a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 index 05a008c48..deecaf6ca 100644 --- a/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 +++ b/Tests/Engine/InvokeScriptAnalyzer.tests.ps1 @@ -143,12 +143,12 @@ Describe "Test IncludeRule" { Context "IncludeRule supports wild card" { It "includes 1 wildcard rule"{ $includeWildcard = Invoke-ScriptAnalyzer $directory\..\Rules\BadCmdlet.ps1 -IncludeRule $avoidRules - $includeWildcard.Count | Should be 3 + $includeWildcard.Count | Should be 0 } it "includes 2 wildcardrules" { $includeWildcard = Invoke-ScriptAnalyzer $directory\..\Rules\BadCmdlet.ps1 -IncludeRule $avoidRules, $useRules - $includeWildcard.Count | Should be 7 + $includeWildcard.Count | Should be 4 } } } @@ -174,12 +174,12 @@ Describe "Test Severity" { It "works with 2 arguments" { $errors = Invoke-ScriptAnalyzer $directory\TestScript.ps1 -Severity Information, Warning - $errors.Count | Should Be 2 + $errors.Count | Should Be 1 } It "works with lowercase argument"{ $errors = Invoke-ScriptAnalyzer $directory\TestScript.ps1 -Severity information, warning - $errors.Count | Should Be 2 + $errors.Count | Should Be 1 } } diff --git a/Tests/Engine/RuleSuppression.tests.ps1 b/Tests/Engine/RuleSuppression.tests.ps1 index de32e5a94..7902648d7 100644 --- a/Tests/Engine/RuleSuppression.tests.ps1 +++ b/Tests/Engine/RuleSuppression.tests.ps1 @@ -50,7 +50,7 @@ Describe "RuleSuppressionWithScope" { Context "FunctionScope" { It "Does not raise violations" { $suppression = $violations | Where-Object {$_.RuleName -eq "PSAvoidUsingPositionalParameters" } - $suppression.Count | Should Be 1 + $suppression.Count | Should Be 0 } }