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
4 tasks
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 <c.bergmeister@gmail.com>
cde7090
to
f2a518f
Compare
…nalyzer into FixConsistentIndentationForPipeWithCommandInside
bergmeister
added a commit
to bergmeister/PSScriptAnalyzer
that referenced
this issue
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 <c.bergmeister@gmail.com> * 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.
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.