ShaunLawrie / PwshSpectreConsole

πŸ‘» PwshSpectreConsole is a PowerShell wrapper for the awesome Spectre.Console library
https://pwshspectreconsole.com/
MIT License
117 stars 7 forks source link

[BUG/REGRESSION] Multiple issues with the new Format-SpectreTable refactor. #20

Closed futuremotiondev closed 9 months ago

futuremotiondev commented 11 months ago

1. Format-SpectreTable no longer renders a generic list containing objects ([System.Collections.Generic.List[Object]]) when passed to the data parameter. (Regression - This used to work perfectly)

$Results1 = [PSCustomObject]@{
    Label   = 'Python 3.13 (64-bit)'
    Version = '3.13'
    Branch  = 'CURRENT'
    Path    = 'C:\Users\futur\AppData\Local\Programs\Python\Python313\python.exe'
    Arch    = '64'
}

$Results2 = [PSCustomObject]@{
    Label   = 'Python 3.12 (64-bit)'
    Version = '3.12'
    Branch  = 'CURRENT'
    Path    = 'C:\Users\futur\AppData\Local\Programs\Python\Python312\python.exe'
    Arch    = '64'
}

$Results3 = [PSCustomObject]@{
    Label   = 'Python 3.11 (64-bit)'
    Version = '3.11'
    Branch  = 'CURRENT'
    Path    = 'C:\Users\futur\AppData\Local\Programs\Python\Python311\python.exe'
    Arch    = '64'
}
$pythonObjectsList = [System.Collections.Generic.List[Object]]@()
$pythonObjectsList.Add($Results1)
$pythonObjectsList.Add($Results2)
$pythonObjectsList.Add($Results3)

# Doesn't work
Format-SpectreTable -Data $pythonObjectsList -Border Square -Color Grey39

This is what is outputted:

Code_61wywcEeG0

It's displaying the properties of the list itself instead of the properties of all the objects contained in the list.

This works as expected:

$pythonObjectsArray = @()
$pythonObjectsArray += $Results1
$pythonObjectsArray += $Results2
$pythonObjectsArray += $Results3

Format-SpectreTable -Data $pythonObjectsArray -Border Square -Color Grey39

Code_DGZ95jvbOd

Can we please re-add support for generic lists and unwrapping the objects within?

2. Arrays with only single values passed to Format-SpectreTable are rendered incorrectly / broken.

In the latest iteration of Format-SpectreTable, it inexplicably renders the 'Length' attribute of each value in the array (Basically, the character count) instead of actually rendering the value. Here's full code to test:

Clear-Host

$Results1 = [PSCustomObject]@{
    Label   = 'Python 3.13 (64-bit)'
    Version = '3.13'
    Branch  = 'CURRENT'
    Path    = 'C:\Users\futur\AppData\Local\Programs\Python\Python313\python.exe'
    Arch    = '64'
}

$Results2 = [PSCustomObject]@{
    Label   = 'Python 3.12 (64-bit)'
    Version = '3.12'
    Branch  = 'CURRENT'
    Path    = 'C:\Users\futur\AppData\Local\Programs\Python\Python312\python.exe'
    Arch    = '64'
}

$Results3 = [PSCustomObject]@{
    Label   = 'Python 3.11 (64-bit)'
    Version = '3.11'
    Branch  = 'CURRENT'
    Path    = 'C:\Users\futur\AppData\Local\Programs\Python\Python311\python.exe'
    Arch    = '64'
}

$pythonObjectsArray = @()
$pythonObjectsArray += $Results1
$pythonObjectsArray += $Results2
$pythonObjectsArray += $Results3

[Array]$pathData = $pythonObjectsArray | ForEach-Object { $_.Path }
[Array]$versionData = $pythonObjectsArray | ForEach-Object { $_.Version }

Write-Host "Path Data Array:" -ForegroundColor Magenta
$pathData

Write-Host "Version Data Array:" -ForegroundColor Magenta
$versionData

Write-Host "Broken Path Data Table:" -ForegroundColor Magenta
Format-SpectreTable -Data $pathData -Border Square -Color Grey39

Write-Host "Broken Version Data Table:" -ForegroundColor Magenta
Format-SpectreTable -Data $versionData -Border Square -Color Grey39

And here's the output of the above code:

Code_QbYcm9qGYe

Just to add, the -Property parameter doesn't help here as there is no property in $pathData or $versionData.

The only workaround I can think of ATM is to construct objects like this:

$pythonObjectsArray = @()
$pythonObjectsArray += $Results1
$pythonObjectsArray += $Results2
$pythonObjectsArray += $Results3

[Array]$versionData = $pythonObjectsArray | ForEach-Object { [PSCustomObject]@{Version = $_.Version} }

Format-SpectreTable -Data $versionData -Border Square -Color Grey39

But this is entirely unnecessary. We should simply just populate the table with the values contained within the array in cases like this.

3. [Kind of a Regression] The -AllowMarkup flag seems counter to how the original .NET table implementation works to me, but I can live with it.

Take a look at this page, and the example within: https://spectreconsole.net/widgets/table

// Create a table
var table = new Table();

// Add some columns
table.AddColumn("Foo");
table.AddColumn(new TableColumn("Bar").Centered());

// Add some rows
table.AddRow("Baz", "[green]Qux[/]");
table.AddRow(new Markup("[blue]Corgi[/]"), new Panel("Waldo"));

// Render the table to the console
AnsiConsole.Write(table);

They don't use an -AllowMarkup flag anywhere. It just accepts and parses the markup. The same behavior is all over the original library. However, I am aware that this is an Opinionated port of the original library, but it kind of feels off.

See this page as well: https://spectreconsole.net/markup Markup is ubiquitous throughout the entire library.

Summary

I haven't had the time to fully go over the new Format-SpectreTable, and I'm sorry if this is coming off as insulting to @trackd as I'm sure he has worked very hard on the new refactor.

But the new function simply just isn't ready for prime time IMHO. I might try a crack at fixing some of these issues, but right now I just don't have the time. A lot of scripts that I wrote which depend on the older Format-SpectreTable are now broken and I've spent a lot of time tonight trying to refactor them to work within this new release.

If there's any chance, @trackd, that you would take a crack at this - and try to possibly resolve some of these issues/edge-cases, it would be hugely appreciated. Again, I'm sorry if it seems like I'm crapping all over your refactor - I'm not. I just want to make sure the module is as robust as possible.

Any thoughts welcome.

trackd commented 11 months ago

i'll take a look at what's going on, seems strange. It should ignore PSCustomObjects and just use the "old" method of showing all properties. unless you inject a PSTypeName indicating that you have created formatdata for the object.

Btw, for 3,

table.AddRow(new Markup("[blue]Corgi[/]"), new Panel("Waldo"));

they do require the use of Markup() which makes it just as opt in as this?

I haven't had the time to fully go over the new Format-SpectreTable, and I'm sorry if this is coming off as insulting to @trackd as I'm sure he has worked very hard on the https://github.com/ShaunLawrie/PwshSpectreConsole/pull/15. But the new function simply just isn't ready for prime time IMHO.

no worries, this is why previews exist :) to find all the edgecases and fix them before stable release.

I have a pretty good idea what's going on i think, ill try and get a fix in tonight or tomorrow.

Edit: btw as a temporary workaround piping objects to the command should work i think, atleast its worth a try.

$data | Format-Spectretable

Not at the computer at the moment so unable to test myself.

futuremotiondev commented 11 months ago

Btw, for 3,

table.AddRow(new Markup("[blue]Corgi[/]"), new Panel("Waldo"));

they do require the use of Markup() which makes it just as opt in as this?

You're right about this. I think I'm fine with the new -AllowMarkup flag. It's starting to bother me less, and probably easier to implement.

no worries, this is why previews exist :) to find all the edgecases and fix them before stable release.

Good stuff! I'm glad I didn't offend.

I have a pretty good idea what's going on i think, ill try and get a fix in tonight or tomorrow.

That would be awesome. I really appreciate all your hard work on the function. I took a look at the source briefly, and I've been going over both Format-SpectreTable and Get-DefaultDisplayMembers to try and familiarize myself with the logic.

If there is anything I can help with, let me know!

trackd commented 10 months ago

Ok, so a couple of things here..

1) this is fixed now.

2) this is a bit strange now when i looked at it a bit closer..

[Array]$pathData = $pythonObjectsList | ForEach-Object { $_.Path }

Makes a string array, i havnt tested if this worked before but i would be a bit surprised?

it would make alot more sense for the code to be

[Array]$pathData = $pythonObjectsList | Select-Object Path
# or 
Format-SpectreTable -Data $pythonObjectsList -Property Path

it's hitting what is essentially the same code as before my change relevant lines its walking the objects psobject.properties.name to add columns to the table, but as its a string array that doesn't really work..

        else {
            Write-Debug 'no formatting found and no properties selected, enumerating psobject.properties.name'
            foreach ($prop in $collector[0].psobject.Properties.Name) {
                if (-Not [String]::IsNullOrEmpty($prop)) {
                    $table.AddColumn($prop) | Out-Null
                }
            }
        }
fst -data $pathData -Debug
DEBUG: getting formatdata for System.Char System.ValueType System.Object
DEBUG: formatData: 0
DEBUG: no formatting found and no properties selected, enumerating psobject.properties.name
╔════════╗
β•‘ Length β•‘
╠════════╣
β•‘ 65     β•‘
β•‘ 65     β•‘
β•‘ 65     β•‘
β•šβ•β•β•β•β•β•β•β•β•

which can be confirmed with $pathdata[0].psobject.properties.name If that did work before im not entirely sure how?

I'm not sure how we should solve this, we check psobject.properties.name so we can add each property to a column.

3) as for the markdown flag, to expand on my earlier response.. Because it requires a different Class to render rather than normal [Spectre.Console.Text] which is used by default in this module, the flag tells the function to use [Spectre.Console.Markup]

                   if($AllowMarkup) {
                        [Markup]::new([String]$cell.Value)
                    } else {
                        [Text]::new([String]$cell.Value)
                    }

i've just commited my fixes, please test them out and see if it works better for you.

In general i would say it is better to use the pipeline to enumerate ie

$data | Format-SpectreTable

Using an array inline with -Data feels a bit anti-powershell? idk, maybe it's just me? I'm aware all of the examples use this approach so we need it to work. but it's essentially not using the begin, process, end as "intended"

you can try this function to see what i mean,

function test {
    [cmdletbinding()]
    param(
        [parameter(ValueFromPipeline)]
        [Object]
        $data
    )
    begin {
        $PSStyle.Foreground.Green
        'Begin'
        $data.count
        $PSStyle.Reset
    }
    process {
        $PSStyle.Foreground.Cyan
        'Process'
        $data.count
        $data.Gettype().FullName
        $PSStyle.Reset
    }
    end {
        $PSStyle.Foreground.Magenta
        'End'
        $data.count
        $PSStyle.Reset
    }
}
# test
Test -data $pythonObjectsList 
$pythonObjectsList | test
ShaunLawrie commented 10 months ago

The markup in the table is pretty neat @fmotion1 I've used it here https://github.com/ShaunLawrie/PwshEc2Tools/tree/main

https://github.com/ShaunLawrie/PwshEc2Tools/assets/13159458/7172da94-a8ac-4329-a15e-3562a6c5b917

I've added a pile of comments on the #21 PR. Thanks for the additions!

futuremotiondev commented 10 months ago

@ShaunLawrie That's freakin' awesome mate. Love it!

futuremotiondev commented 10 months ago

Using an array inline with -Data feels a bit anti-powershell? idk, maybe it's just me? I'm aware all of the examples use this approach so we need it to work. but it's essentially not using the begin, process, end as "intended"

Personally I do it all the time...

I completely understand that a major signature of PowerShell is pipeline processing and I'm completely on board when it comes to ensuring the pipeline works as intended with this function. But please don't neglect those of us that do use functions in alternative ways without large pipeline chains. Sometimes it's way more readable to write code like this.

Thanks again for all your changes though, I am going to install the latest prerelease now and give all the new changes a whirl!

trackd commented 10 months ago

But please don't neglect those of us that do use functions in alternative ways without large pipeline chains. Sometimes it's way more readable to write code like this.

Oh, i didn't mean to imply that I was ignoring your issue, it should absolutely work and it does now.

Thanks again for all your changes though, I am going to install the latest prerelease now and give all the new changes a whirl!

my pr is not yet merged, I'm going to try and implement a few more changes to solve your second point, and do a bit of restructure before marking the pr ready.

if 1. is a big blocker you could grab Format-SpectreTable in my pr. Hoping to have the other stuff added tonight or tomorrow.

trackd commented 10 months ago

image

function ConvertTo-SpectreRainbow {
    param(
        [Parameter(Mandatory, ValueFromPipeline)]
        [String]$string
    )
    begin {
        $spectreColors = [Spectre.Console.Color] | Get-Member -Static -Type Properties | Select-Object -ExpandProperty Name
    }
    process {
        $String.EnumerateRunes() | ForEach-Object {
            "[{0}]$($_.ToString())[/]" -f $spectreColors[(Get-Random -Minimum 0 -Maximum $spectreColors.Count)]
        } | Join-String
    }
}

kind of fun to play around with ;)

ShaunLawrie commented 10 months ago

Haha nice, if you convert the color to HSV and sort by hue you can do a smooth-ish rainbow.

I'm keen to get your format-spectretable changes merged to prerelease.

I'm on holiday without a laptop so it will be stuck in prerelease until I'm back in 2.5 weeks. The changes in prerelease have been pretty significant to add the testability and I was thinking of bumping the spectre console version that it's using before merging it to main and publishing a new minor version.

trackd commented 10 months ago

Haha nice, if you convert the color to HSV and sort by hue you can do a smooth-ish rainbow.

that is smoother, the silly things are the most fun πŸ˜† image

function ConvertTo-SpectreRainbow {
    param(
        [Parameter(Mandatory, ValueFromPipeline)]
        [String]$string
    )
    begin {
        function Get-SpectreHsv {
            [CmdletBinding()]
            param()
            $ColorNames = [Spectre.Console.Color] | Get-Member -Static -Type Properties | Select-Object -ExpandProperty Name
            foreach ($Name in $ColorNames) {
                $Spectrecolor = [Spectre.Console.Color]::$Name
                $drawing = [System.Drawing.Color]::FromArgb("0x$($Spectrecolor.ToHex())")
                [PSCustomObject]@{
                    Name       = $Name
                    Hue        = $drawing.GetHue()
                    Saturation = $drawing.GetSaturation()
                    Brightness = $drawing.GetBrightness()
                }
            }
        }
        $spectreColors = (Get-SpectreHsv | Sort-Object Hue).Name
        $i = 0
    }
    process {
        $String.EnumerateRunes() | ForEach-Object {
            "[{0}]$($_.ToString())[/]" -f $spectreColors[$i]
            if ($i -eq ($spectreColors.Count - 1)) {
                $i = 0
            }
            else {
                $i++
            }
        } | Join-String
    }
}

I'm keen to get your format-spectretable changes merged to prerelease.

yea that would be cool, the pr is getting a bit big now and a bit expanded from its original purpose :)

i did sneak in a new public function πŸ˜… (visible in the improved rainbowtable ) hint πŸ“†

I'm on holiday without a laptop so it will be stuck in prerelease until I'm back in 2.5 weeks. The changes in prerelease have been pretty significant to add the testability and I was thinking of bumping the spectre console version that it's using before merging it to main and publishing a new minor version.

πŸ‘
i've been been testing out one of the 0.48.1 preview releases for a while now so i think bumping Spectre.Console version should be fine.

ShaunLawrie commented 10 months ago

Cool, bump it in your branch and I’ll hit merge to prerelease when you say it’s ready

On Thu, 18 Jan 2024 at 1:26β€―PM, trackd @.***> wrote:

Haha nice, if you convert the color to HSV and sort by hue you can do a smooth-ish rainbow.

that is smoother, the silly things are the most fun πŸ˜† image.png (view on web) https://github.com/ShaunLawrie/PwshSpectreConsole/assets/17672644/b4312169-dc01-4ba3-bdb2-a0efcedaec3d

function ConvertTo-SpectreRainbow { param( [Parameter(Mandatory, ValueFromPipeline)] [String]$string ) begin { function Get-SpectreHsv { [CmdletBinding()] param() $ColorNames = [Spectre.Console.Color] | Get-Member -Static -Type Properties | Select-Object -ExpandProperty Name foreach ($Name in $ColorNames) { $Spectrecolor = [Spectre.Console.Color]::$Name $drawing = [System.Drawing.Color]::FromArgb("0x$($Spectrecolor.ToHex())") [PSCustomObject]@{ Name = $Name Hue = $drawing.GetHue() Saturation = $drawing.GetSaturation() Brightness = $drawing.GetBrightness() } } } $spectreColors = (Get-SpectreHsv | Sort-Object Hue).Name $i = 0 } process { $String.EnumerateRunes() | ForEach-Object { "[{0}]$($_.ToString())[/]" -f $spectreColors[$i] if ($i -eq ($spectreColors.Count - 1)) { $i = 0 } else { $i++ } } | Join-String } }

I'm keen to get your format-spectretable changes merged to prerelease.

yea that would be cool, the pr is getting a bit big now and a bit expanded from its original purpose :)

i did sneak in a new public function πŸ˜… (visible in the improved rainbowtable ) hint πŸ“†

I'm on holiday without a laptop so it will be stuck in prerelease until I'm back in 2.5 weeks. The changes in prerelease have been pretty significant to add the testability and I was thinking of bumping the spectre console version that it's using before merging it to main and publishing a new minor version.

πŸ‘ i've been been testing out one of the 0.48.1 preview releases for a while now so i think bumping Spectre.Console version should be fine.

β€” Reply to this email directly, view it on GitHub https://github.com/ShaunLawrie/PwshSpectreConsole/issues/20#issuecomment-1897548045, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADEMYIXDLSYZKMOOXHQEGXDYPBT2LAVCNFSM6AAAAABBJ3VBY6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJXGU2DQMBUGU . You are receiving this because you were mentioned.Message ID: @.***>

ShaunLawrie commented 9 months ago

I think the issues in here are all addressed in the latest prerelease. Is that right @fmotion1 ?