Skip to content

/ PSScriptAnalyzer

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

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

Open
wants to merge 9 commits into
base: master
from
Open

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

wants to merge 9 commits into from

Conversation

@bergmeister
Copy link
Collaborator

bergmeister commented May 3, 2020 •
edited

PR Summary

Fixes #1407 cc @felixbecker

This is to fix a long-standing bug in the formatter that has been present probably since day 1.
There are a few cases that were similar to this one where an LParen and LBrace are both on the same line cause an increase of indentation that is twice as much as needed.

(foo | bar {
    baz
})

Therefore this PR adds a check to see if the LParen is the first token on a line by checking the previous token is of kind Newline. Then it also checks that the following token (comments have to be skipped) is also not a Newline so that the following case would still indent:

$result = (
    Get-Something
).Property

In order to prevent the RParen code from de-indenting too much, we have to keep a stack of when we skipped the indentation caused by tokens that require a closing RParen (which are LParen, AtParen and DollarParen).

PR Checklist
@bergmeister
Do not increase indentation after LParen the previous token is a Newl…
Loading status checks…
8a7847e 
…ine and the next token is not a Newline
@bergmeister bergmeister changed the title Do not increase indentation after LParen the previous token is a Newline and the next token is not a Newline Do not increase indentation after LParen if the previous token is a Newline and the next token is not a Newline May 3, 2020
bergmeister added 3 commits May 3, 2020
@bergmeister
TryPop not availabe in full .net. Also cleanup code
Loading status checks…
20c110a
@bergmeister
add test cases
Loading status checks…
194022d
@bergmeister
cleanup code comment
Loading status checks…
bd738a7
@bergmeister bergmeister marked this pull request as ready for review May 3, 2020
Christoph Bergmeister
re-trigger ci
Loading status checks…
607615c
@rjmholt rjmholt requested review from rjmholt and JamesWTruher Jul 16, 2020
@rjmholt
rjmholt reviewed Jul 21, 2020
View changes
Rules/UseConsistentIndentation.cs
case TokenKind.LCurly:
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);

This comment has been minimized.

Sign in to view
@rjmholt

rjmholt Jul 21, 2020 Member

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

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Jul 21, 2020 Author Collaborator

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.

This comment has been minimized.

Sign in to view
@rjmholt

rjmholt Jul 21, 2020 Member

I'm happy with it as is
Rules/UseConsistentIndentation.cs
case TokenKind.DollarParen:
case TokenKind.AtParen:
lParenSkippedIndentation.Push(false);
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine);

This comment has been minimized.

Sign in to view
@rjmholt

rjmholt Jul 21, 2020 Member

ditto
Rules/UseConsistentIndentation.cs Outdated Show resolved Hide resolved
Rules/UseConsistentIndentation.cs
if (lParenSkippedIndentation.Count > 0)
{
matchingLParenIncreasedIndentation = lParenSkippedIndentation.Pop();
}

This comment has been minimized.

Sign in to view
@rjmholt

rjmholt Jul 21, 2020 Member

Suggested change
}
}

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Jul 21, 2020 Author Collaborator

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.

This comment has been minimized.

Sign in to view
@rjmholt

rjmholt Jul 21, 2020 Member

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

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Jul 21, 2020 •
edited
Author Collaborator

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.
Rules/UseConsistentIndentation.cs Show resolved Hide resolved
Rules/UseConsistentIndentation.cs Outdated Show resolved Hide resolved
Rules/UseConsistentIndentation.cs Outdated Show resolved Hide resolved
Rules/UseConsistentIndentation.cs Outdated Show resolved Hide resolved
pssa-1.19.0-blog.md Outdated Show resolved Hide resolved
Christoph Bergmeister added 4 commits Jul 21, 2020
Christoph Bergmeister
Merge branch 'master' of https://github.com/powershell/psscriptanalyzer…
5fd495c 
… into issue1407
Christoph Bergmeister
remove accidentally checked in pssa 1.19.0 blog
Loading status checks…
a47555f
Christoph Bergmeister
Simplify 'if (onNewLine) { onNewLine = false; }' to 'onNewLine = fals…
Loading status checks…
5f2201e 
…e;' and fix spelling in comment
Christoph Bergmeister
- Simplify token checking code and add comment around stack
Loading status checks…
b3bf36d 
- Remove accidentally checked in file
@rjmholt
rjmholt approved these changes Jul 22, 2020
View changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjmholt rjmholt

@JamesWTruher JamesWTruher

Assignees
No one assigned
Labels
Area - Formatter
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

PSUseConsistentIndentation: Indentation wrong if pipeline is wrapped in parentheses
2 participants
@bergmeister @rjmholt
You can’t perform that action at this time.