Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 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 @@ -25,6 +25,7 @@
PSUseConsistentIndentation = @{
Enable = $true
Kind = 'space'
PipelineIndentation = 'IncreaseIndentationAfterEveryPipeline'
IndentationSize = 4
}

Expand Down
1 change: 1 addition & 0 deletions Engine/Settings/CodeFormattingAllman.psd1
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
PSUseConsistentIndentation = @{
Enable = $true
Kind = 'space'
PipelineIndentation = 'IncreaseIndentationAfterEveryPipeline'
IndentationSize = 4
}

Expand Down
1 change: 1 addition & 0 deletions Engine/Settings/CodeFormattingOTBS.psd1
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
PSUseConsistentIndentation = @{
Enable = $true
Kind = 'space'
PipelineIndentation = 'IncreaseIndentationAfterEveryPipeline'
IndentationSize = 4
}

Expand Down
1 change: 1 addition & 0 deletions Engine/Settings/CodeFormattingStroustrup.psd1
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
PSUseConsistentIndentation = @{
Enable = $true
Kind = 'space'
PipelineIndentation = 'IncreaseIndentationAfterEveryPipeline'
IndentationSize = 4
}

Expand Down
24 changes: 24 additions & 0 deletions RuleDocumentation/UseConsistentIndentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ Indentation should be consistent throughout the source file.
PSUseConsistentIndentation = @{
Enable = $true
IndentationSize = 4
PipelineIndentation = 'IncreaseIndentationForFirstPipeline'
Kind = 'space'
}
}
Expand All @@ -30,6 +31,29 @@ Enable or disable the rule during ScriptAnalyzer invocation.

Indentation size in the number of space characters.

#### PipelineIndentation: string (Default value is `IncreaseIndentationForFirstPipeline`)

Whether to increase indentation after a pipeline for multi-line statements. The settings are:

- IncreaseIndentationForFirstPipeline (default): Indent once after the first pipeline and keep this indentation. Example:
```powershell
foo |
bar |
baz
```
- IncreaseIndentationAfterEveryPipeline: Indent more after the first pipeline and keep this indentation. Example:
```powershell
foo |
bar |
baz
```
- NoIndentation: Do not increase indentation. Example:
```powershell
foo |
bar |
baz
```

#### Kind: string (Default value is `space`)

Represents the kind of indentation to be used. Possible values are: `space`, `tab`. If any invalid value is given, the property defaults to `space`.
Expand Down
93 changes: 83 additions & 10 deletions Rules/UseConsistentIndentation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,24 @@ public string Kind
}
}


[ConfigurableRuleProperty(defaultValue: "IncreaseIndentationForFirstPipeline")]
public string PipelineIndentation
{
get
{
return pipelineIndentationStyle.ToString();
}
set
{
if (String.IsNullOrWhiteSpace(value) ||
!Enum.TryParse(value, true, out pipelineIndentationStyle))
{
pipelineIndentationStyle = PipelineIndentationStyle.IncreaseIndentationAfterEveryPipeline;
}
}
}

private bool insertSpaces;
private char indentationChar;
private int indentationLevelMultiplier;
Expand All @@ -68,9 +86,17 @@ private enum IndentationKind {
// Auto
};

private enum PipelineIndentationStyle
{
IncreaseIndentationForFirstPipeline,
IncreaseIndentationAfterEveryPipeline,
NoIndentation
}

// TODO make this configurable
private IndentationKind indentationKind = IndentationKind.Space;

private PipelineIndentationStyle pipelineIndentationStyle = PipelineIndentationStyle.IncreaseIndentationAfterEveryPipeline;

/// <summary>
/// Analyzes the given ast to find violations.
Expand Down Expand Up @@ -104,6 +130,7 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
var diagnosticRecords = new List<DiagnosticRecord>();
var indentationLevel = 0;
var onNewLine = true;
var pipelineAsts = ast.FindAll(testAst => testAst is PipelineAst && (testAst as PipelineAst).PipelineElements.Count > 1, true);
for (int k = 0; k < tokens.Length; k++)
{
var token = tokens[k];
Expand All @@ -123,6 +150,27 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);
break;

case TokenKind.Pipe:
var pipelineIsFollowedByNewlineOrLineContinuation = k < tokens.Length - 1 && k > 0 &&
(tokens[k + 1].Kind == TokenKind.NewLine || tokens[k + 1].Kind == TokenKind.LineContinuation);
if (!pipelineIsFollowedByNewlineOrLineContinuation)
{
break;
}
if (pipelineIndentationStyle == PipelineIndentationStyle.IncreaseIndentationAfterEveryPipeline)
{
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);
}
else if (pipelineIndentationStyle == PipelineIndentationStyle.IncreaseIndentationForFirstPipeline)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use break above and eliminate else here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this would probably be more natural as a switch

Copy link
Collaborator Author

@bergmeister bergmeister Mar 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the proposed break statement in the first if statement to convert the else if to an if statement in this commit. A switch statement for 2 cases (out of 3 enum values) is not worth the horizontal space IMHO and does not feel more readable to me (as a reader, I expect a switch statement to go through all enum values). I had to force-push once because the git pull -r that I had to do due to the committed suggestions, messed up the branch so I had to revert that and pull again normally with a clean merge commit. Shall I merge now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall I merge now?

LGTM

{
var isFirstPipeInPipeline = pipelineAsts.Any(pipelineAst => PositionIsEqual(((PipelineAst)pipelineAst).PipelineElements[0].Extent.EndScriptPosition, tokens[k - 1].Extent.EndScriptPosition));
if (isFirstPipeInPipeline)
{
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);
}
}
break;

case TokenKind.RParen:
case TokenKind.RCurly:
indentationLevel = ClipNegative(indentationLevel - 1);
Expand Down Expand Up @@ -156,22 +204,44 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
{
--j;
}

if (j >= 0 && tokens[j].Kind == TokenKind.Pipe)
{
++tempIndentationLevel;
}
}

AddViolation(token, tempIndentationLevel, diagnosticRecords, ref onNewLine);
var lineHasPipelineBeforeToken = tokens.Any(oneToken =>
oneToken.Kind == TokenKind.Pipe &&
oneToken.Extent.StartLineNumber == token.Extent.StartLineNumber &&
oneToken.Extent.StartColumnNumber < token.Extent.StartColumnNumber);

AddViolation(token, tempIndentationLevel, diagnosticRecords, ref onNewLine, lineHasPipelineBeforeToken);
}
break;
}

// Check if the current token matches the end of a PipelineAst
var matchingPipeLineAstEnd = pipelineAsts.FirstOrDefault(pipelineAst =>
PositionIsEqual(pipelineAst.Extent.EndScriptPosition, token.Extent.EndScriptPosition)) as PipelineAst;
if (matchingPipeLineAstEnd != null)
{
if (pipelineIndentationStyle == PipelineIndentationStyle.IncreaseIndentationForFirstPipeline)
{
indentationLevel = ClipNegative(indentationLevel - 1);
}
else if (pipelineIndentationStyle == PipelineIndentationStyle.IncreaseIndentationAfterEveryPipeline)
{
indentationLevel = ClipNegative(indentationLevel - (matchingPipeLineAstEnd.PipelineElements.Count - 1));
}
}
}

return diagnosticRecords;
}

private static bool PositionIsEqual(IScriptPosition position1, IScriptPosition position2)
{
return position1.ColumnNumber == position2.ColumnNumber &&
position1.LineNumber == position2.LineNumber &&
position1.File == position2.File;
}

/// <summary>
/// Retrieves the common name of this rule.
/// </summary>
Expand Down Expand Up @@ -237,7 +307,8 @@ private void AddViolation(
Token token,
int expectedIndentationLevel,
List<DiagnosticRecord> diagnosticRecords,
ref bool onNewLine)
ref bool onNewLine,
bool lineHasPipelineBeforeToken = false)
{
if (onNewLine)
{
Expand Down Expand Up @@ -265,26 +336,28 @@ private void AddViolation(
GetDiagnosticSeverity(),
fileName,
null,
GetSuggestedCorrections(token, expectedIndentationLevel)));
GetSuggestedCorrections(token, expectedIndentationLevel, lineHasPipelineBeforeToken)));
}
}
}

private List<CorrectionExtent> GetSuggestedCorrections(
Token token,
int indentationLevel)
int indentationLevel,
bool lineHasPipelineBeforeToken = false)
{
// TODO Add another constructor for correction extent that takes extent
// TODO handle param block
// TODO handle multiline commands

var corrections = new List<CorrectionExtent>();
var optionalPipeline = lineHasPipelineBeforeToken ? "| " : string.Empty;
corrections.Add(new CorrectionExtent(
token.Extent.StartLineNumber,
token.Extent.EndLineNumber,
1,
token.Extent.EndColumnNumber,
GetIndentationString(indentationLevel) + token.Extent.Text,
GetIndentationString(indentationLevel) + optionalPipeline + token.Extent.Text,
token.Extent.File));
return corrections;
}
Expand Down
Loading