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

Remove redundant whitespace between parameters with new option (disabled by default) in UseConsistentWhitespace #1392

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Engine/Settings/CodeFormatting.psd1
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
CheckOperator = $true
CheckPipe = $true
CheckSeparator = $true
CheckParameter = $false
}

PSAlignAssignmentStatement = @{
Expand Down
1 change: 1 addition & 0 deletions Engine/Settings/CodeFormattingAllman.psd1
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
CheckOperator = $true
CheckPipe = $true
CheckSeparator = $true
CheckParameter = $false
}

PSAlignAssignmentStatement = @{
Expand Down
1 change: 1 addition & 0 deletions Engine/Settings/CodeFormattingOTBS.psd1
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
CheckOperator = $true
CheckPipe = $true
CheckSeparator = $true
CheckParameter = $false
}

PSAlignAssignmentStatement = @{
Expand Down
1 change: 1 addition & 0 deletions Engine/Settings/CodeFormattingStroustrup.psd1
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
CheckOperator = $true
CheckPipe = $true
CheckSeparator = $true
CheckParameter = $false
}

PSAlignAssignmentStatement = @{
Expand Down
6 changes: 6 additions & 0 deletions RuleDocumentation/UseConsistentWhitespace.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
CheckOperator = $true
CheckPipe = $true
CheckSeparator = $true
CheckParameter = $false
}
}
```
Expand Down Expand Up @@ -53,3 +54,8 @@ Checks if a comma or a semicolon is followed by a space. E.g. `@(1, 2, 3)` or `@
#### CheckPipe: bool (Default value is `$true`)

Checks if a pipe is surrounded on both sides by a space. E.g. `foo | bar` instead of `foo|bar`.

#### CheckParameter: bool (Default value is `$false` at the moment due to the setting being new)

Checks if there is more than one space between parameters and values. E.g. `foo -bar $baz -bat` instead of `foo -bar $baz -bat`. This eliminates redundant whitespace that was probably added unintentionally.
The rule does not check for whitespace between parameter and value when the colon syntax `-ParameterName:$ParameterValue` is used as some users prefer either 0 or 1 whitespace in this case.
89 changes: 45 additions & 44 deletions Rules/Strings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion Rules/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -1104,4 +1104,7 @@
<data name="UseProcessBlockForPipelineCommandName" xml:space="preserve">
<value>UseProcessBlockForPipelineCommand</value>
</data>
</root>
<data name="UseConsistentWhitespaceErrorSpaceBetweenParameter" xml:space="preserve">
<value>Use only 1 whitespace between parameter names or values.</value>
</data>
</root>
53 changes: 52 additions & 1 deletion Rules/UseConsistentWhitespace.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
#endif
public class UseConsistentWhitespace : ConfigurableRule
{
private enum ErrorKind { BeforeOpeningBrace, Paren, Operator, SeparatorComma, SeparatorSemi, AfterOpeningBrace, BeforeClosingBrace, BeforePipe, AfterPipe };
private enum ErrorKind { BeforeOpeningBrace, Paren, Operator, SeparatorComma, SeparatorSemi,
AfterOpeningBrace, BeforeClosingBrace, BeforePipe, AfterPipe, BetweenParameter };
private const int whiteSpaceSize = 1;
private const string whiteSpace = " ";
private readonly SortedSet<TokenKind> openParenKeywordWhitelist = new SortedSet<TokenKind>()
Expand Down Expand Up @@ -56,6 +57,9 @@ private List<Func<TokenOperations, IEnumerable<DiagnosticRecord>>> violationFind
[ConfigurableRuleProperty(defaultValue: true)]
public bool CheckSeparator { get; protected set; }

[ConfigurableRuleProperty(defaultValue: true)]
public bool CheckParameter { get; protected set; }

public override void ConfigureRule(IDictionary<string, object> paramValueMap)
{
base.ConfigureRule(paramValueMap);
Expand Down Expand Up @@ -110,6 +114,11 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
diagnosticRecords = diagnosticRecords.Concat(violationFinder(tokenOperations));
}

if (CheckParameter)
{
diagnosticRecords = diagnosticRecords.Concat(FindParameterViolations(ast));
}

return diagnosticRecords.ToArray(); // force evaluation here
}

Expand Down Expand Up @@ -203,6 +212,8 @@ private string GetError(ErrorKind kind)
return string.Format(CultureInfo.CurrentCulture, Strings.UseConsistentWhitespaceErrorSeparatorComma);
case ErrorKind.SeparatorSemi:
return string.Format(CultureInfo.CurrentCulture, Strings.UseConsistentWhitespaceErrorSeparatorSemi);
case ErrorKind.BetweenParameter:
return string.Format(CultureInfo.CurrentCulture, Strings.UseConsistentWhitespaceErrorSpaceBetweenParameter);
default:
return string.Format(CultureInfo.CurrentCulture, Strings.UseConsistentWhitespaceErrorBeforeParen);
}
Expand Down Expand Up @@ -364,6 +375,46 @@ private IEnumerable<DiagnosticRecord> FindOpenParenViolations(TokenOperations to
}
}

private IEnumerable<DiagnosticRecord> FindParameterViolations(Ast ast)
{
IEnumerable<Ast> commandAsts = ast.FindAll(
testAst => testAst is CommandAst, true);
foreach (CommandAst commandAst in commandAsts)
{
List<Ast> commandParameterAstElements = commandAst.FindAll(testAst => true, searchNestedScriptBlocks: false).ToList();
for (int i = 0; i < commandParameterAstElements.Count - 1; i++)
{
IScriptExtent leftExtent = commandParameterAstElements[i].Extent;
IScriptExtent rightExtent = commandParameterAstElements[i + 1].Extent;
if (leftExtent.EndLineNumber != rightExtent.StartLineNumber)
{
continue;
}
bergmeister marked this conversation as resolved.
Show resolved Hide resolved

var expectedStartColumnNumberOfRightExtent = leftExtent.EndColumnNumber + 1;
bergmeister marked this conversation as resolved.
Show resolved Hide resolved
if (rightExtent.StartColumnNumber > expectedStartColumnNumberOfRightExtent)
bergmeister marked this conversation as resolved.
Show resolved Hide resolved
{
int numberOfRedundantWhiteSpaces = rightExtent.StartColumnNumber - expectedStartColumnNumberOfRightExtent;
var correction = new CorrectionExtent(
startLineNumber: leftExtent.StartLineNumber,
endLineNumber: leftExtent.EndLineNumber,
startColumnNumber: leftExtent.EndColumnNumber + 1,
endColumnNumber: leftExtent.EndColumnNumber + 1 + numberOfRedundantWhiteSpaces,
text: string.Empty,
file: leftExtent.File);

yield return new DiagnosticRecord(
GetError(ErrorKind.BetweenParameter),
leftExtent,
GetName(),
GetDiagnosticSeverity(),
leftExtent.File,
suggestedCorrections: new CorrectionExtent[] { correction });
}
}
}
}

private bool IsSeparator(Token token)
{
return token.Kind == TokenKind.Comma || token.Kind == TokenKind.Semi;
Expand Down
66 changes: 66 additions & 0 deletions Tests/Rules/UseConsistentWhitespace.tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ $ruleConfiguration = @{
CheckOperator = $false
CheckPipe = $false
CheckSeparator = $false
CheckParameter = $false
}

$settings = @{
Expand Down Expand Up @@ -388,4 +389,69 @@ if ($true) { Get-Item `
}
}


Context "CheckParameter" {
BeforeAll {
$ruleConfiguration.CheckInnerBrace = $true
$ruleConfiguration.CheckOpenBrace = $false
$ruleConfiguration.CheckOpenParen = $false
$ruleConfiguration.CheckOperator = $false
$ruleConfiguration.CheckPipe = $false
$ruleConfiguration.CheckSeparator = $false
$ruleConfiguration.CheckParameter = $true
}

It "Should not find no violation when newlines are involved" {
$def = {foo -a $b `
-c d -d $e -f g `
-h i |
bar -h i `
-switch}
Invoke-ScriptAnalyzer -ScriptDefinition "$def" -Settings $settings | Should -Be $null
}

It "Should not find no violation if there is always 1 space between parameters except when using colon syntax" {
bergmeister marked this conversation as resolved.
Show resolved Hide resolved
$def = 'foo -bar $baz @splattedVariable -bat -parameterName:$parameterValue'
Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings | Should -Be $null
}

It "Should find 1 violation if there is 1 space too much before a parameter" {
$def = 'foo -bar'
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
$violations.Count | Should -Be 1
$violations[0].Extent.Text | Should -Be 'foo'
$violations[0].SuggestedCorrections[0].Text | Should -Be ([string]::Empty)
}

It "Should find 1 violation if there is 1 space too much before a parameter value" {
$def = 'foo $bar'
$violations = Invoke-ScriptAnalyzer -ScriptDefinition $def -Settings $settings
$violations.Count | Should -Be 1
$violations[0].Extent.Text | Should -Be 'foo'
$violations[0].SuggestedCorrections[0].Text | Should -Be ([string]::Empty)
}

It "Should fix script to always have 1 space between parameters except when using colon syntax but not by default" {
$def = 'foo -bar $baz -ParameterName: $ParameterValue'
Invoke-Formatter -ScriptDefinition $def |
Should -BeExactly $def -Because 'CheckParameter configuration is not turned on by default (yet) as the setting is new'
Invoke-Formatter -ScriptDefinition $def -Settings $settings |
Should -BeExactly 'foo -bar $baz -ParameterName: $ParameterValue'
}

It "Should fix script when newlines are involved" {
$def = {foo -a $b `
-c d -d $e -f g `
-h i |
bar -h i `
-switch}
$expected = {foo -a $b `
-c d -d $e -f g `
-h i |
bar -h i `
-switch}
Invoke-Formatter -ScriptDefinition "$def" -Settings $settings |
Should -Be "$expected"
}
}
}