-
Notifications
You must be signed in to change notification settings - Fork 383
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
Conversation
…ine and the next token is not a Newline
case TokenKind.LCurly: | ||
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
case TokenKind.DollarParen: | ||
case TokenKind.AtParen: | ||
lParenSkippedIndentation.Push(false); | ||
AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
if (lParenSkippedIndentation.Count > 0) | ||
{ | ||
matchingLParenIncreasedIndentation = lParenSkippedIndentation.Pop(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
…e;' and fix spelling in comment
- Remove accidentally checked in file
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
andLBrace
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 kindNewline
. Then it also checks that the following token (comments have to be skipped) is also not aNewline
so that the following case would still indent: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 closingRParen
(which areLParen
,AtParen
andDollarParen
).PR Checklist
.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
to the beginning of the title and remove the prefix when the PR is ready.