Skip to content

Formatter fails for multi-line pipelines when the first line has two or more pipes #1459

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

Closed
EklipZgit opened this issue Apr 22, 2020 · 4 comments · Fixed by #1463
Closed

Comments

@EklipZgit
Copy link

Issue Description

Under extension settings, set Pipeline Indentation Style to IncreaseIndentationForFirstPipeline (which I think is a more sensible default than No Indentation though I don't know what the official community guidelines say on the topic...).

Then format the following:

# arbitrary code flow structure to create nesting for bug demo
if ($true)
{
    foreach ($i in 0..10)
    {
        $works = $something.Value | 
            Group-Object -NoElement | 
            Where-Object { $_.Count -gt 2 } |
            Sort-Object -Property Count -Descending |
            Select-Object -First 1 -ExpandProperty Name

        $andThisWorks = $something.Value | 
            Group-Object -NoElement | Where-Object { $_.Count -gt 1 } |   # doubled pipeline on non-first line
            Sort-Object -Property Count -Descending |
            Select-Object -First 1 -ExpandProperty Name

        $broken = $something.Value | Group-Object -NoElement | # doubled pipeline on first line
            Where-Object { $_.Count -gt 1 } |
            Sort-Object -Property Count -Descending |
            Select-Object -First 1 -ExpandProperty Name

        $norDoesTripleWork = $something.Value | Group-Object -NoElement | Where-Object { $_.Count -gt 1 } |   # tripled pipeline on non-first line
            Sort-Object -Property Count -Descending |
            Select-Object -First 1 -ExpandProperty Name
    }
}

Output is

# arbitrary code flow structure to create nesting for bug demo
if ($true)
{
    foreach ($i in 0..10)
    {
        $works = $something.Value | 
            Group-Object -NoElement | 
            Where-Object { $_.Count -gt 2 } |
            Sort-Object -Property Count -Descending |
            Select-Object -First 1 -ExpandProperty Name

        $andThisWorks = $something.Value | 
            Group-Object -NoElement | Where-Object { $_.Count -gt 1 } | # doubled pipeline on non-first line
            Sort-Object -Property Count -Descending |
            Select-Object -First 1 -ExpandProperty Name

        $broken = $something.Value | Group-Object -NoElement | # doubled pipeline on first line
        Where-Object { $_.Count -gt 1 } |
        Sort-Object -Property Count -Descending |
        Select-Object -First 1 -ExpandProperty Name

    $norDoesTripleWork = $something.Value | Group-Object -NoElement | Where-Object { $_.Count -gt 1 } | # tripled pipeline on non-first line
    Sort-Object -Property Count -Descending |
    Select-Object -First 1 -ExpandProperty Name
}
}

Attached Logs

Follow the instructions in the README about
capturing and sending logs.

Environment Information

Visual Studio Code

Name Version
Operating System Windows_NT x64 10.0.16299
VSCode 1.44.1
PowerShell Extension Version 2020.4.2

PowerShell Information

Name Value
PSVersion 5.1.16299.1146
PSEdition Desktop
PSCompatibleVersions 1.0 2.0 3.0 4.0 5.0 5.1.16299.1146
BuildVersion 10.0.16299.1146
CLRVersion 4.0.30319.42000
WSManStackVersion 3.0
PSRemotingProtocolVersion 2.3
SerializationVersion 1.1.0.1

Visual Studio Code Extensions

Visual Studio Code Extensions(Click to Expand)
Extension Author Version
azurerm-vscode-tools msazurermtools 0.9.0
githistory donjayamanne 0.6.3
gitlens eamodio 10.2.1
powershell-preview ms-vscode 2020.4.2
remote-wsl ms-vscode-remote 0.44.2
@rjmholt rjmholt transferred this issue from PowerShell/vscode-powershell Apr 23, 2020
@ghost ghost added the Needs: Triage 🔍 label Apr 23, 2020
@bergmeister
Copy link
Collaborator

@EklipZgit Thanks for the detailed report. Some fixes and improvements have been made after the release of the current version 1.18.3. With the current state of master the formatted output doesn't have the wrong indentation of the second last brace any more shown in your output but it is still missing the expected, additional indentation in the last 2 examples:

if ($true)
{
    foreach ($i in 0..10)
    {
        $works = $something.Value | 
            Group-Object -NoElement | 
            Where-Object { $_.Count -gt 2 } |
            Sort-Object -Property Count -Descending |
            Select-Object -First 1 -ExpandProperty Name

        $andThisWorks = $something.Value | 
            Group-Object -NoElement | Where-Object { $_.Count -gt 1 } |   # doubled pipeline on non-first line
            Sort-Object -Property Count -Descending |
            Select-Object -First 1 -ExpandProperty Name

        $broken = $something.Value | Group-Object -NoElement | # doubled pipeline on first line
        Where-Object { $_.Count -gt 1 } |
        Sort-Object -Property Count -Descending |
        Select-Object -First 1 -ExpandProperty Name

        $norDoesTripleWork = $something.Value | Group-Object -NoElement | Where-Object { $_.Count -gt 1 } |   # tripled pipeline on non-first line
        Sort-Object -Property Count -Descending |
        Select-Object -First 1 -ExpandProperty Name
    }
}

Formatting with the IncreaseIndentationAfterEveryPipeline setting shows that it does generally managed to increase the indentation, therefore it seems that it only has problems doing that for the 2nd line as well.

if ($true)
{
    foreach ($i in 0..10)
    {
        $works = $something.Value |
            Group-Object -NoElement |
                Where-Object { $_.Count -gt 2 } |
                    Sort-Object -Property Count -Descending |
                        Select-Object -First 1 -ExpandProperty Name

        $andThisWorks = $something.Value |
            Group-Object -NoElement | Where-Object { $_.Count -gt 1 } |   # doubled pipeline on non-first line
            Sort-Object -Property Count -Descending |
                Select-Object -First 1 -ExpandProperty Name

        $broken = $something.Value | Group-Object -NoElement | # doubled pipeline on first line
        Where-Object { $_.Count -gt 1 } |
            Sort-Object -Property Count -Descending |
                Select-Object -First 1 -ExpandProperty Name

        $norDoesTripleWork = $something.Value | Group-Object -NoElement | Where-Object { $_.Count -gt 1 } |   # tripled pipeline on non-first line
        Sort-Object -Property Count -Descending |
            Select-Object -First 1 -ExpandProperty Name
    }
}

The reason why the default setting of powershell.codeFormatting.pipelineIndentationStyle is NoIndentation is to be closer to the behaviour before this new setting/feature was introduced as this new feature still shows some special cases where indentation does not work as expected as you see but your feedback is very appreciated to make it better. I've tried to fix various cases in the last releases and will continue to so that in the near future (hopefully be end of this year), we could change the default to something like IncreaseIndentationForFirstPipeline. The next release of PSSA 1.19.0 is coming out in the next weeks and as part of it the default will change to a new setting option called None as there were even a few special cases that did not work with NoIndentation, I will try to fix your reported cases for the release if I can. Therefore the message is to be patient for the moment but keep reporting cases that don't work as expected.

@bergmeister
Copy link
Collaborator

bergmeister commented Apr 26, 2020

@EklipZgit Some investigation shows that if the trailing comment in the last examples gets removed, then the formatter indents correctly. Therefore the only case we have to fix is:

foo | # comment
bar

I opened a PR for it to fix that (see link below)

@EklipZgit
Copy link
Author

EklipZgit commented Apr 30, 2020

I've tried to fix various cases in the last releases and will continue to so that in the near future (hopefully be end of this year), we could change the default to something like IncreaseIndentationForFirstPipeline

@bergmeister I dunno if this is the place for this discussion, but for what it's worth I would expect the default behavior to not touch these, quite honestly. I have always double indented mine as a LINQ indentation style i brought with me from C#, and I find it troubling that not only is the double-indent not an option (fine, I'm not standard, can't really complain about that), but none of the options will just leave it alone. Personally, leave-it-alone should be the default behavior imo for personal-preference style stuff like this. If we bothered multi-lining a pipeline then it probably already looks the way we want it to look ;)
Hopefully this is what your new 'None' setting you mentioned will do, as default.

Absolute worst case is automatically de-indenting everybodies nicely indented pipeline continuations like it does now though, so I would definitely settle with IncreaseIndentationForFirstPipeline if "don't auto format multi-line pipelines" isn't an acceptable default.

@bergmeister
Copy link
Collaborator

@EklipZgit Yes, the plan is to have None as default with the next version, which doesn't touch indentation at all, 1.19.0 is going to release soon. You can test-drive it from here already but it will take a few weeks for the GA release and until it makes it into the VS-Code PowerShell extension (preview will be first)

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