Skip to content
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

Format indention of multiline statements #1315

Open
msftrncs opened this issue Aug 21, 2019 · 12 comments
Open

Format indention of multiline statements #1315

msftrncs opened this issue Aug 21, 2019 · 12 comments

Comments

@msftrncs
Copy link

Summary of the new feature

Enhance formatting of continued (multiline) statements (command and expression), combining the PipeLineIndentionStyle with statement continuation.

There are a lot of means to continue command statements, including pipe lines. especially so with expressions. I, myself, prefer all continued statements to be indented, but currently the formatting only supports indenting when a brace or parenthesis is involved, or optionally with a pipe line.

Proposed technical implementation details (optional)

I actually think the details of pipe line continuation gets simpler when the entire statement's continuation is taken in to account.

I am not fully understanding of how the formatting mechanism works, so this is a fundamental concept to how I would think it would work.

If using PowerShell AST's, for each PipeLineAst/CommandExpressionAst/etc, add 1 indention level to subsequent lines, in addition to all other indenting even for the same PipeLineAst.

There might be a need to restrict the continuation indention of a PipeLine or CommandExpression if they are the sole contents of a ParenExpression, because in that case, the PipeLine or CommandExpression is fully captured in the indention of the ParenExpression, as technically only one expression should appear in such cases, unlike a scriptblock or statementblock where multiple statements or expressions could appear.

I need to gather some examples to better demonstrate my use case.

@bergmeister
Copy link
Collaborator

Yesterday I just got a prototype in my branch IndentationOfMulitlineCommandWithCommentInBetween working to fix multi line command with line continuation tokens (i.e. backtick) when there is a comment line in the middle of those. I will open a PR soon.

Otherwise PSSA already has the new PipelineIndentation feature (already exposed in VSCode but not enabled by default yet as there are still a few edge cases to be ironed out) for the use cases that you describe but maybe an example could help clarify if I understand correctly what you mean.
The formatter rules are all Token based btw, they can compare a token to the Ast though if necessary

@bergmeister
Copy link
Collaborator

Please see PR #1318 for scenario that I fixed today and issue #1317 for issues that I found that are to be fixed.

@msftrncs
Copy link
Author

Note that when I mention PipeLine, I am referring to a AST type, for which almost every command and expression result in, even when no | is present.

Going to try to get some examples started:

(Current formatter, irrespectively of pipe indention setting)

return $_parseerrors.Count -ne 0 -or $_tokens.Count -ne ($tokenToCheck + 2) -or $_tokens[$tokenToCheck].Kind -in (
    [TokenKind]::Variable,
    [TokenKind]::SplattedVariable,
    [TokenKind]::StringExpandable,
    [TokenKind]::StringLiteral,
    [TokenKind]::HereStringExpandable,
    [TokenKind]::HereStringLiteral,
    [TokenKind]::Comment) -or ($IsExpandable -and $_tokens[1] -is [StringExpandableToken]) -or
($_tokens[$tokenToCheck] -is [StringToken] -and $_tokens[$tokenToCheck].Value.Length -ne $in.Length) -or
$_tokens[$tokenToCheck + 1].Kind -ne [TokenKind]::EndOfInput

(Expected)

return $_parseerrors.Count -ne 0 -or $_tokens.Count -ne ($tokenToCheck + 2) -or $_tokens[$tokenToCheck].Kind -in (
        [TokenKind]::Variable,
        [TokenKind]::SplattedVariable,
        [TokenKind]::StringExpandable,
        [TokenKind]::StringLiteral,
        [TokenKind]::HereStringExpandable,
        [TokenKind]::HereStringLiteral,
        [TokenKind]::Comment) -or ($IsExpandable -and $_tokens[1] -is [StringExpandableToken]) -or
    ($_tokens[$tokenToCheck] -is [StringToken] -and $_tokens[$tokenToCheck].Value.Length -ne $in.Length) -or
    $_tokens[$tokenToCheck + 1].Kind -ne [TokenKind]::EndOfInput

@bergmeister
Copy link
Collaborator

bergmeister commented Aug 23, 2019

Thanks for the example, very useful.
I agree that the last 2 lines should be corrected to have 1 level of indentation.
But why do you expect 2 levels of indentation, for the first part? I'd expect only 1. Only if the open paren on line1 was a brace then I'd understand it.
Compare that to C#, it'd only add one line of indentation if you added a newline after after Combine:

        private bool foo()
        {
            string a = "a";
            string b = "b";
            string c = "c";
            return (a != null && System.IO.Path.Combine(a, b, c) == "a/b/c");
        }

@msftrncs
Copy link
Author

msftrncs commented Aug 23, 2019

The two levels of indent on the second line makes the last two lines being indented 1 level make more sense. The reason all the lines are expected to be indented 1 level more than they currently do, is because the 'pipeline' (also AssignmentExpression or ReturnStatement (but a PipeLine immediately following a ReturnStatement doesn't add additional indent)) is still open at the start of each line.

Note, a ; here would act just like a ) or }, if used in the first column, it will be un-indented, because it marks the end of the pipeline. Of course, an additional pipeline may form right after it. However, a ; on a new line would be unusual..

test line `
    <# comment indented, as pipeline is continued #>`
; <# pipeline ended #> Start NewCommand `
    <# new pipeline continued here #>
Normal Indent

I did notice that the C# formatter will preserve the indention of continued lines.

Technically, PowerShell AST's have the 'fat' trimmed off, so in my example above, the comments do not participate in the PipeLineAst, which means they wouldn't format as expected. Effectively the line continuation is ignored, because nothing meaningful continued the line.

So, if I understand this right:

The formatter rules are all Token based btw

Using tokens will take a little more thought as to how to detect command/expression line continuation. Technically, all StatementBlock based statements can have continuation as well (naturally, compared to commands/expressions).

switch 
    -file 'filename'
{
    CondA
    {
        Statements ConditionA
    }
}

The indenting would be the same with OTBS. This is because the parameter/condition portion of the statement ends when the { begins, nullifying a change in indention.

Another note on the logic of this style of formatting: If each line possessed a Boolean property, IndentNextLineDueToContinuation, this property can only be set once per line, so at most only 1 level of indent can be added per line due to it being continued. This is in contrast to indention for { or (. Even though a line could technically possess multiple commands/expressions that are being continued, nested commands/expressions would be enclosed in either { or ( already. It is possible that a count of the number of open commands/expressions would be needed to be tracked.

@msftrncs
Copy link
Author

msftrncs commented Aug 24, 2019

Another example from my code base:

(current formatter)

(Get-Item 'aFilePathWildCard*').foreach{
    Get-ChildItem $_
}.where{
    $_.Name -match '^\p{L}\w*,'
}.foreach{
    [pscustomobject]@{
        LastName  = $_.Name.Remove($_.Name.IndexOf(', '))
        FirstName = $_.BaseName.SubString($_.Name.IndexOf(', ') + 2)
        File      = $_
    }
}

(expected)

(Get-Item 'aFilePathWildCard*').foreach{
        Get-ChildItem $_
    }.where{
        $_.Name -match '^\p{L}\w*,'
    }.foreach{
        [pscustomobject]@{
                LastName  = $_.Name.Remove($_.Name.IndexOf(', '))
                FirstName = $_.BaseName.SubString($_.Name.IndexOf(', ') + 2)
                File      = $_
            }
    }

@msftrncs
Copy link
Author

msftrncs commented Aug 27, 2019

I just rewrote my prior example, and it gives another variation. The first sample of the set actually formats as expected with the PipelineIndention option, unless I merge the first two lines. My point is that the specifics of the line wrapping (like merging the first two lines) should not affect indention.

Actual current with IncreaseIndentationForFirstPipeline:

Get-Item '.\Statements ????' |
    Get-ChildItem -File | Where-Object {  $_.Name -match '^\p{L}\w*,' } | ForEach-Object {
        [pscustomobject]@{
             Year      = $_.Directory.Name.SubString($_.Directory.Name.LastIndexOf(' ') + 1)
             LastName  = $_.Name.Remove($_.Name.IndexOf(', '))
             FirstName = $_.BaseName.SubString($_.Name.IndexOf(', ') + 2)
             File      = $_
        }
    }

or

Get-Item '.\Statements ????' | Get-ChildItem -File | Where-Object {  $_.Name -match '^\p{L}\w*,' } | ForEach-Object {
    [pscustomobject]@{
         Year      = $_.Directory.Name.SubString($_.Directory.Name.LastIndexOf(' ') + 1)
         LastName  = $_.Name.Remove($_.Name.IndexOf(', '))
         FirstName = $_.BaseName.SubString($_.Name.IndexOf(', ') + 2)
         File      = $_
    }
}

Expected:

Get-Item '.\Statements ????' |
    Get-ChildItem -File | Where-Object {  $_.Name -match '^\p{L}\w*,' } | ForEach-Object {
        [pscustomobject]@{
                 Year      = $_.Directory.Name.SubString($_.Directory.Name.LastIndexOf(' ') + 1)
                 LastName  = $_.Name.Remove($_.Name.IndexOf(', '))
                 FirstName = $_.BaseName.SubString($_.Name.IndexOf(', ') + 2)
                 File      = $_
            }
    }

or

Get-Item '.\Statements ????' | Get-ChildItem -File | Where-Object {  $_.Name -match '^\p{L}\w*,' } | ForEach-Object {
        [pscustomobject]@{
                 Year      = $_.Directory.Name.SubString($_.Directory.Name.LastIndexOf(' ') + 1)
                 LastName  = $_.Name.Remove($_.Name.IndexOf(', '))
                 FirstName = $_.BaseName.SubString($_.Name.IndexOf(', ') + 2)
                 File      = $_
            }
    }

@msftrncs
Copy link
Author

I've revised my expected results above, regarding hashtable assignments, as I missed that those are really just pipeline expressions, so according to what I propose, they would receive the double indentation as well.

@LethiferousMoose
Copy link

LethiferousMoose commented Jan 18, 2022

If the formatter is token based, does mean the following format for PowerShell class is not possible?

class TestClass {
    [void] MethodWithLongArgs([string] $argumentOne, [string] $argumentTwo,
            [string] $argumentThree, [string] $argumentFour) {
        [string] $stringVarWithLongAssignment =
            'this is a very long string that needs to be wrapped onto the next line'
    }
}

Note the backtick is not required for this type of line continuation.

Currently formatting results in this:

class TestClass {
    [void] MethodWithLongArgs([string] $argumentOne, [string] $argumentTwo,
        [string] $argumentThree, [string] $argumentFour) {
        [string] $stringVarWithLongAssignment =
        'this is a very long string that needs to be wrapped onto the next line'
    }
}

@bergmeister
Copy link
Collaborator

Formatting rules are token based, although the related AST can be looked up. Pretty much everything is possible but tweaking special cases like the one you showed for classes, take significant effort. Things slowly do get better so pointing out those cases is still helpful but it naturally takes time to address them all.

@LethiferousMoose
Copy link

Is there a running backlog of "planned" formatting improvements?

@bergmeister
Copy link
Collaborator

Is there a running backlog of "planned" formatting improvements?

The issues in this repo, I don't think they are well tagged but most probably have the Area - Formatter tag, some of them I assigned to myself for personal tracking. High impact ones or easy ones I tend to fix as soon as I see them, the others I pick up whenever I have spare time as I am a community contributor. Unfortunately there is not much capacity from the Microsoft side, most of it is either dedicated to certain features and also supporting me by reviewing PRs, maintaining their internal build that is used for publishing PSSA and the publishing itself. I am on Github Sponsors here if you want to help support me: https://github.com/sponsors/bergmeister/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants