GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Do not increase indentation after LParen if the previous token is a Newline and the next token is not a Newline by bergmeister · Pull Request #1469 · PowerShell/PSScriptAnalyzer Conversation
|
| case TokenKind.AtParen: | ||
| case TokenKind.LParen: | ||
| case TokenKind.LCurly: | ||
| AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine); |
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with it as is
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| case TokenKind.DollarParen: | ||
| case TokenKind.AtParen: | ||
| lParenSkippedIndentation.Push(false); | ||
| AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
ditto
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| if (lParenSkippedIndentation.Count > 0) | ||
| { | ||
| matchingLParenIncreasedIndentation = lParenSkippedIndentation.Pop(); | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| } | |
| } | |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…e;' and fix spelling in comment
- Remove accidentally checked in file
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
PSUseConsistentIndentation: Indentation wrong if pipeline is wrapped in parentheses
There was an error while loading. Please reload this page.