Fix PSUseConsistentIndentationRule to handle pipes correctly when there is a multi-line statement after a pipe and add PipelineIndentation customisation option for it #1102
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…re is a multi-line statement after it: Do not just add temporary indentation for the next line but keep the indentation level
bergmeister
changed the title
bergmeister
changed the title
bergmeister
requested review from
kapilmb and
JamesWTruher
and removed request for
kapilmb
This was referenced Dec 20, 2018
bergmeister
added
the
(Re-)Review Needed
11 tasks
rjmholt
approved these changes
Mar 5, 2019
Rules/UseConsistentIndentation.cs
Outdated
| { | ||
| AddViolation(token, indentationLevel++, diagnosticRecords, ref onNewLine); | ||
| } | ||
| else if (pipelineIndentationStyle == PipelineIndentationStyle.IncreaseIndentationForFirstPipeline) |
There was a problem hiding this comment.
Can use break above and eliminate else here
There was a problem hiding this comment.
Although this would probably be more natural as a switch
There was a problem hiding this comment.
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?
Co-Authored-By: bergmeister <[email protected]>
bergmeister
force-pushed
the
FixConsistentIndentationForPipeWithCommandInside
branch
from
cde7090 to
f2a518f
Compare
…nalyzer into FixConsistentIndentationForPipeWithCommandInside
bergmeister
added a commit
to bergmeister/PSScriptAnalyzer
that referenced
this pull request
Mar 22, 2019
…re is a multi-line statement after a pipe and add PipelineIndentation customisation option for it (PowerShell#1102) * Fix PSUseConsistentIndentationRule to handle pipes correctly when there is a multi-line statement after it: Do not just add temporary indentation for the next line but keep the indentation level * correctly decrement indentation count for multiple pipes * add customisation * take pipeline into account and add docs. TODO: Change shipped setting files * update formatting setting files * add tests and refactor/improve test suite * Apply suggestions from code review Co-Authored-By: bergmeister <[email protected]> * Remove else block by using break
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Summary
Fixes #1066
Do not just add temporary indentation for the next line but keep the indentation level for the next lines and then decrement it, this allows to handle multiline statements.
This adds the new option
PipelineIndentationwith the 3 options:This PR also fixes a small bug with the auto-correction if a pipe is on the same line where indentation has to be corrected (regression test case added) and improves the test suite
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.