Skip to content

Do not increase indentation after LParen if the previous token is a Newline and the next token is not a Newline #1469

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Nov 10, 2020
Merged

Do not increase indentation after LParen if the previous token is a Newline and the next token is not a Newline #1469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8a7847e
Do not increase indentation after LParen the previous token is a Newl…
bergmeister May 3, 2020
20c110a
TryPop not availabe in full .net. Also cleanup code
bergmeister May 3, 2020
194022d
add test cases
bergmeister May 3, 2020
bd738a7
cleanup code comment
bergmeister May 3, 2020
607615c
re-trigger ci
May 4, 2020
5fd495c
Merge branch 'master' of https://github.com/powershell/psscriptanalyz…
Jul 21, 2020
a47555f
remove accidentally checked in pssa 1.19.0 blog
Jul 21, 2020
5f2201e
Simplify 'if (onNewLine) { onNewLine = false; }' to 'onNewLine = fals…
Jul 21, 2020
b3bf36d
- Simplify token checking code and add comment around stack
Jul 22, 2020
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
65 changes: 63 additions & 2 deletions Rules/UseConsistentIndentation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,13 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
var onNewLine = true;
var pipelineAsts = ast.FindAll(testAst => testAst is PipelineAst && (testAst as PipelineAst).PipelineElements.Count > 1, true).ToList();
int minimumPipelineAstIndex = 0;
/*
When an LParen and LBrace are on the same line, it can lead to too much de-indentation.
In order to prevent the RParen code from de-indenting too much, we keep a stack of when we skipped the indentation
caused by tokens that require a closing RParen (which are LParen, AtParen and DollarParen).
*/
var lParenSkippedIndentation = new Stack<bool>();

for (int tokenIndex = 0; tokenIndex < tokens.Length; tokenIndex++)
{
var token = tokens[tokenIndex];
Expand All @@ -146,10 +153,27 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
switch (token.Kind)
{
case TokenKind.AtCurly:
case TokenKind.AtParen:
case TokenKind.LParen:
case TokenKind.LCurly:
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would break the ++ here out into a preceding statement. Even though the C# language spec guarantees the order of evaluation of arguments (not the case in other languages), it's still more explicit to embed the side-effect as its own statement above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's 3 other places in this file where indentationLevel++ is passed into a method and similar to your other style suggestion here, I think we should rather have a PR after that to apply style changes consistently. Unfortunately the code has already too much vertical method length for my taste, therefore I personally don't see the value given the price of one more line but that is not a strong opinion and would still accept your proposed style changes if you feel it's worthwhile.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy with it as is

break;

case TokenKind.DollarParen:
case TokenKind.AtParen:
lParenSkippedIndentation.Push(false);
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

break;

case TokenKind.LParen:
// When a line starts with a parenthesis and it is not the last non-comment token of that line,
// then indentation does not need to be increased.
if ((tokenIndex == 0 || tokens[tokenIndex - 1].Kind == TokenKind.NewLine) &&
NextTokenIgnoringComments(tokens, tokenIndex)?.Kind != TokenKind.NewLine)
{
onNewLine = false;
lParenSkippedIndentation.Push(true);
break;
}
lParenSkippedIndentation.Push(false);
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);
break;

Expand Down Expand Up @@ -181,6 +205,20 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
break;

case TokenKind.RParen:
bool matchingLParenIncreasedIndentation = false;
if (lParenSkippedIndentation.Count > 0)
{
matchingLParenIncreasedIndentation = lParenSkippedIndentation.Pop();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This would make it inconsistent with the existing style where there is one newline only between the many case statements, which makes it nice to read in logically separated blocks of reasonable size IMHO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well the current PSSA codebase style is such that I won't argue. I think blocks should always be separated by newlines (and would have included another newline below the if below if I'd noticed), but not willing to spend too much energy on it

Copy link
Collaborator Author

@bergmeister bergmeister Jul 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not a fan of the PSSA style either TBH (especially the long methods where high level context is needed). I suggest a follow-up style PR to make the whole file consistent with this style or define something like an editorconfig file so that we could apply the preferred format across all files automatically.

if (matchingLParenIncreasedIndentation)
{
onNewLine = false;
break;
}
indentationLevel = ClipNegative(indentationLevel - 1);
AddViolation(token, indentationLevel, diagnosticRecords, ref onNewLine);
break;

case TokenKind.RCurly:
indentationLevel = ClipNegative(indentationLevel - 1);
AddViolation(token, indentationLevel, diagnosticRecords, ref onNewLine);
Expand Down Expand Up @@ -259,6 +297,29 @@ public override IEnumerable<DiagnosticRecord> AnalyzeScript(Ast ast, string file
return diagnosticRecords;
}

private static Token NextTokenIgnoringComments(Token[] tokens, int startIndex)
{
if (startIndex >= tokens.Length - 1)
{
return null;
}

for (int i = startIndex + 1; i < tokens.Length; i++)
{
switch (tokens[i].Kind)
{
case TokenKind.Comment:
continue;

default:
return tokens[i];
}
}

// We've run out of tokens
return null;
}

private static bool PipelineIsFollowedByNewlineOrLineContinuation(Token[] tokens, int startIndex)
{
if (startIndex >= tokens.Length - 1)
Expand Down
66 changes: 66 additions & 0 deletions Tests/Rules/UseConsistentIndentation.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,72 @@ $param3
}
}

Context 'LParen indentation' {
It 'Should preserve script when line starts with LParen' {
$IdempotentScriptDefinition = @'
function test {
(foo | bar {
baz
})
Do-Something
}
'@
Invoke-Formatter -ScriptDefinition $IdempotentScriptDefinition | Should -Be $idempotentScriptDefinition
}

It 'Should preserve script when line starts and ends with LParen' {
$IdempotentScriptDefinition = @'
function test {
(
foo | bar {
baz
}
)
Do-Something
}
'@
Invoke-Formatter -ScriptDefinition $IdempotentScriptDefinition | Should -Be $idempotentScriptDefinition
}

It 'Should preserve script when line starts and ends with LParen but trailing comment' {
$IdempotentScriptDefinition = @'
function test {
( # comment
foo | bar {
baz
}
)
Do-Something
}
'@
Invoke-Formatter -ScriptDefinition $IdempotentScriptDefinition | Should -Be $idempotentScriptDefinition
}

It 'Should preserve script when there is Newline after LParen' {
$IdempotentScriptDefinition = @'
function test {
$result = (
Get-Something
).Property
Do-Something
}
'@
Invoke-Formatter -ScriptDefinition $IdempotentScriptDefinition | Should -Be $idempotentScriptDefinition
}

It 'Should preserve script when there is a comment and Newline after LParen' {
$IdempotentScriptDefinition = @'
function test {
$result = ( # comment
Get-Something
).Property
Do-Something
}
'@
Invoke-Formatter -ScriptDefinition $IdempotentScriptDefinition | Should -Be $idempotentScriptDefinition
}
}

Context "When a sub-expression is provided" {
It "Should not find a violations" {
$def = @'
Expand Down