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

Bug - Multiple TableRowEntry Piped to Write-FormatTableView No Label #235

Open
mattcargile opened this issue Jul 1, 2024 · 9 comments
Open
Labels

Comments

@mattcargile
Copy link

mattcargile commented Jul 1, 2024

Description

When piping [pscustomobject]s and binding by property name, the Label node is missing on TableColumnHeader causing certain formatting issues like the Label ends up being the plain text of the [scriptblock].

I'd expect the Label to match name in -Property [array] if no AliasProperty. I assume the last [pscustomobject] in the [array] will determine the TableColumnHeader structure.

Workaround

Use -AliasProperty @{ Name = 'Name'; Mode = 'Mode'} on the last [pscustomobject] in the @().

Notes

I don't think Write-FormatListView shows this same behavior. Loving getting my formatting set up in my build.ps1. Thanks for the time on this!

Reproducable Steps

Install-PSResource PowershellHumanizer, Terminal-Icons
$formatList = [System.Collections.Generic.List[string]]::new()
$grpSetCtrlName = 'FileSystemTypes-GroupingFormat'
$writeFormatCustomViewSplat = @{
    Name = $grpSetCtrlName
    AsControl = $true
    Frame = $true
    LeftIndent = 4
    Action = {
        Write-FormatViewExpression -AssemblyName 'System.Management.Automation' -BaseName 'FileSystemProviderStrings' -ResourceID 'DirectoryDisplayGrouping'
        Write-FormatViewExpression -ScriptBlock {$_.PSParentPath.Replace("Microsoft.PowerShell.Core\FileSystem::", "")}
        Write-FormatViewExpression -Newline
    }
}
$formatList.Add( (Write-FormatCustomView @writeFormatCustomViewSplat) )
$writeFormatDeserDirInfoNamespace = 'Deserialized.System.IO.DirectoryInfo'
$writeFormatDeserFileInfoNamespace = 'Deserialized.System.IO.FileInfo'
$writeFormatDirInfoNamespace = 'System.IO.DirectoryInfo'
$writeFormatFileInfoNamespace = 'System.IO.FileInfo'
$writeFormatSelSetName = 'FileSystemTypes'
$writeFormatDeserSelSetName = 'FileSystemTypesDeserialized'
$writeFormatViewSplat = @{
    FormatXml = @"
        <SelectionSet>
            <Name>$writeFormatSelSetName</Name>
            <Types>
                <TypeName>$writeFormatDirInfoNamespace</TypeName>
                <TypeName>$writeFormatFileInfoNamespace</TypeName>
                <TypeName>$writeFormatDeserDirInfoNamespace</TypeName>
                <TypeName>$writeFormatDeserFileInfoNamespace</TypeName>
            </Types>
        </SelectionSet>
"@
    TypeName  = 'NotApplicable'
}
$formatList.Add( (Write-FormatView @writeFormatViewSplat) )
$writeFormatViewSplat = @{
    FormatXml = @"
        <SelectionSet>
            <Name>$writeFormatDeserSelSetName</Name>
            <Types>
                <TypeName>$writeFormatDeserDirInfoNamespace</TypeName>
                <TypeName>$writeFormatDeserFileInfoNamespace</TypeName>
            </Types>
        </SelectionSet>
"@
    TypeName  = 'NotApplicable'
}
$formatList.Add( (Write-FormatView @writeFormatViewSplat) )

$writeFormatWidth = [Int32[]]@(7, 25, 14)
$writeFormatAlign = @{
    Mode          = 'Left'
    LastWriteTime = 'Right'
    Length        = 'Right'
    Name          = 'Left'
}
$writeFormatLengthSb = {
    if ($_.Attributes.HasFlag( [System.IO.FileAttributes]::Offline) ) {
        "($([Humanizer.ByteSizeExtensions]::Humanize($_.Length, '0.00')))"
    }
    else {
        [Humanizer.ByteSizeExtensions]::Humanize($_.Length, '0.00')
    }
}
$writeFormatFileAndDirInfoProperty = 'ModeWithoutHardLink', 'LastWriteTime', 'Length', 'Name'
$writeFormatFileAndDirInfoAliasProperty = @{
    ModeWithoutHardLink = 'Mode'
}
$writeFormatDeserFileAndDirInfoProperty = 'Mode', 'LastWriteTime', 'Length', 'Name'

$writeFormatTable = @(
    [PSCustomObject]@{
        Property        = $writeFormatFileAndDirInfoProperty
        AlignProperty   = $writeFormatAlign
        Width           = $writeFormatWidth 
        Wrap            = $true
        VirtualProperty = @{
            LastWriteTime = { [Humanizer.DateHumanizeExtensions]::Humanize($_.LastWriteTime, $false) }
            Length        = $writeFormatLengthSb 
            Name          = { Terminal-Icons\Format-TerminalIcons $_ }
        }
        AliasProperty   = $writeFormatFileAndDirInfoAliasProperty
    },
    [PSCustomObject]@{
        ViewTypeName    = $writeFormatDeserDirInfoNamespace
        Property        = $writeFormatDeserFileAndDirInfoProperty
        AlignProperty   = $writeFormatAlign
        Width           = $writeFormatWidth 
        Wrap            = $true
        VirtualProperty = @{
            LastWriteTime = { [Humanizer.DateHumanizeExtensions]::Humanize($_.LastWriteTime, $false) }
            Length        = { [string]::Empty }
        }
    },
    [PSCustomObject]@{
        ViewTypeName    = $writeFormatDirInfoNamespace
        Property        = $writeFormatFileAndDirInfoProperty 
        AlignProperty   = $writeFormatAlign
        Width           = $writeFormatWidth 
        Wrap            = $true
        VirtualProperty = @{
            LastWriteTime = { [Humanizer.DateHumanizeExtensions]::Humanize($_.LastWriteTime, $false) }
            Length        = { [string]::Empty }
            Name          = { Terminal-Icons\Format-TerminalIcons $_ }
        }
        AliasProperty   = $writeFormatFileAndDirInfoAliasProperty
    },
    [PSCustomObject]@{
        ViewTypeName    = $writeFormatDeserFileInfoNamespace
        Property        = $writeFormatDeserFileAndDirInfoProperty 
        AlignProperty   = $writeFormatAlign
        Width           = $writeFormatWidth 
        Wrap            = $true
        VirtualProperty = @{
            LastWriteTime = { [Humanizer.DateHumanizeExtensions]::Humanize($_.LastWriteTime, $false) }
            Length        = { [Humanizer.ByteSizeExtensions]::Humanize($_.Length, '0.00') }
        }
    }
)
$writeFormatViewSplat = @{
    FormatXml       = ( $writeFormatTable | Write-FormatTableView )
    IsSelectionSet  = $true
    Name            = 'children'
    TypeName        = $writeFormatSelSetName
    GroupByProperty = 'PSParentPath'
    GroupAction     = $grpSetCtrlName
}
$formatList.Add( ( Write-FormatView @writeFormatViewSplat ) )

$file = "$env:USERPROFILE\mattcargile_235.format.ps1xml"
$formatList | Out-FormatData | Set-Content $file -Confirm
Import-Module PowerShellHumanizer, Terminal-Icons
Update-FormatData -PrependPath $file
Get-ChildItem # Look at the header where Name would be. You will see the [scriptblock]
Remove-Item $file -Confirm

image

Links

N/A

@mattcargile mattcargile added the bug label Jul 1, 2024
@StartAutomating
Copy link
Owner

@mattcargile

I appreciate your interest in EZOut and its implementation. I'm also at a bit of a loss for exactly what your expectations are for the codeblock above?

What are you expecting? As designed - I would expect Write-FormatTableView to rarely be called directly. (it's more commonly called within Write-FormatView, often within a .format.ps1 file).

If you were to pipe in multiple items as it stands, I believe the lack of a corresponding -Condition would make the -ViewTypeName be effectively ignored.

Perhaps this is partially because is a different from what I'd consider a "normal" implementation of EZOut (see the /Types or /Formats directory structure in many modules, including this one).

Perhaps this is because I'm missing some context about this scenario.

I'm happy to hop on a call to figure out what's going on.

Please provide a little more context / repro so we can get things going.

@mattcargile
Copy link
Author

@mattcargile

I appreciate your interest in EZOut and its implementation. I'm also at a bit of a loss for exactly what your expectations are for the codeblock above?

What are you expecting? As designed - I would expect Write-FormatTableView to rarely be called directly. (it's more commonly called within Write-FormatView, often within a .format.ps1 file).

If you were to pipe in multiple items as it stands, I believe the lack of a corresponding -Condition would make the -ViewTypeName be effectively ignored.

Perhaps this is partially because is a different from what I'd consider a "normal" implementation of EZOut (see the /Types or /Formats directory structure in many modules, including this one).

Perhaps this is because I'm missing some context about this scenario.

I'm happy to hop on a call to figure out what's going on.

Please provide a little more context / repro so we can get things going.

Well that is embarrassing, James. I am very sorry as I am seeing I provided a terrible example. :-)

I've since updated my Issue with a full repro and a screenshot of the issue. I'm doing it this way so I can have multiple TableRowEntry for Deserialized types and for DirectoryInfo versus FileInfo. I figured it might be faster this way versus trying to add an if ( $_ -is [DirectoryInfo] ) { } check inside the ScriptBlock node.

@StartAutomating
Copy link
Owner

@mattcargile thanks for the additional context. A few thoughts and questions remain.

I'm still not quite sure why you'd do this in multiple entries. FWIW, I think at this point it is pretty clear that $_ -is [IO.DirectoryInfo] is at least logically much more clear (and it should be pretty darn fast).

I wonder if the fact that it's named Name is a cause for weirdness. I'll try to debug later to see if I can spot what's truly wrong.

Additional high-level implementation thoughts below:

-AliasProperties aren't exactly a good idea (in formatters alone)

This was recognized pretty early on, when people saw the formatting for Get-Process and could not access the properties that the table suggested existed.

In general, I believe it's better to create an AliasProperty in a .types.ps1xml, too. This way, the property actually exists when the user goes looking for it (instead of wondering while $file.Mode returns blank)

This is super easy. Just create a directory for your type, and then make an Alias.psd1 file. The key is the alias, the value is what it is aliased to. Then run Import-TypeView on that directory and you'll be good to go.

EZOut works best with modules

It might be my bad in documentation, but I believe you really should be using EZOut within a module structure.

Here's a step-by-step "get up and running" checklist.

  1. Write-EZFormatFile -ModuleName $MyModuleName -OutputPath $MyModule.ezout.ps1
  2. Make a /Types directory
  3. Make a subdirectory beneath this for each type in the module
  4. .format.ps1 and .view.ps1 files beneath these directories will become .format.ps1xml
  5. Everything else will become a .types.ps1xml

The more I've dealt with both types and formatters, the more I've come to see and respect that types files are the more useful of the pair.

Last but not least

It seems like you're making a nice humanized file formatter. This is great, and if you wanted to, you could just modify the formatter in Posh to support a humanized form of the file access time. There's already code in there to allow use of terminal-icons, so, why not add support for conditional humanization, too?

Point is: whenever you've got it sorted enough, I'd happily take a PR for Posh.

@mattcargile
Copy link
Author

mattcargile commented Jul 4, 2024

I'm still not quite sure why you'd do this in multiple entries. FWIW, I think at this point it is pretty clear that $_ -is [IO.DirectoryInfo] is at least logically much more clear (and it should be pretty darn fast).

I want Deserialized "file" objects to be formatted as well. My biggest issue is that pwsh Core broke icm server1 { ls } formatting. The Name and Length are blank. Desktop accounted for Deserialized objects.

I thought it would make the logic simpler with four different entries especially since the xml design seems built to handle different [type]s. It seemed like a shame to build in if logic into the [scriptblock] when there was a ready-made abstraction at the xml layer.

Additionally, Terminal-Icons doesn't accept the Deserialized objects and the Attributes property doesn't have a HasFlag method to check for OneDrive Offline files.

I wonder if the fact that it's named Name is a cause for weirdness. I'll try to debug later to see if I can spot what's truly wrong.

I don't think so. You can see Mode isn't shown in the table header node either. It works because there isn't a [scriptblock].

Additional high-level implementation thoughts below:

-AliasProperties aren't exactly a good idea (in formatters alone)

This was recognized pretty early on, when people saw the formatting for Get-Process and could not access the properties that the table suggested existed.

In general, I believe it's better to create an AliasProperty in a .types.ps1xml, too. This way, the property actually exists when the user goes looking for it (instead of wondering while $file.Mode returns blank)

This is super easy. Just create a directory for your type, and then make an Alias.psd1 file. The key is the alias, the value is what it is aliased to. Then run Import-TypeView on that directory and you'll be good to go.

I'd agree with this sentiment too. The alias properties aren't really aliases, it is just a way to force the headers to work. The only "real" use of the Alias Propery is to make use of the ModeWithoutHardlink which is a hidden string property that has to be -Force-ed to see anyway and I am "aliasing" it as a real property in Mode anyway. This is the native functionality in Core which is a little deceptive though I wanted to keep the defaults.

EZOut works best with modules

It might be my bad in documentation, but I believe you really should be using EZOut within a module structure.

Here's a step-by-step "get up and running" checklist.

  1. Write-EZFormatFile -ModuleName $MyModuleName -OutputPath $MyModule.ezout.ps1
  2. Make a /Types directory
  3. Make a subdirectory beneath this for each type in the module
  4. .format.ps1 and .view.ps1 files beneath these directories will become .format.ps1xml
  5. Everything else will become a .types.ps1xml

The more I've dealt with both types and formatters, the more I've come to see and respect that types files are the more useful of the pair.

I've got a profile.psm1 file and I may decide to incorporate the EzOut module feature. This was my first step though. Also, I am not keen on adding types to FileInfo. I don't need any alias properties right now.

Last but not least

It seems like you're making a nice humanized file formatter. This is great, and if you wanted to, you could just modify the formatter in Posh to support a humanized form of the file access time. There's already code in there to allow use of terminal-icons, so, why not add support for conditional humanization, too?

Point is: whenever you've got it sorted enough, I'd happily take a PR for Posh.

I did look at that formatter while I was creating my build.ps1. I really want my Deserialized objects to work for FileInfo which I suppose could be implemented in Posh. I think the bigger issue is my dependency on PowerShellHumanizer which would probably slow Posh's load down too much.

P.S. I'll see if I can hook up the debugger and play with it too. I assume the issue is the process block deriving the table header based on the last object passed through.

@StartAutomating
Copy link
Owner

I want Deserialized "file" objects to be formatted as well. My biggest issue is that pwsh Core broke icm server1 { ls } formatting. The Name and Length are blank. Desktop accounted for Deserialized objects.

Deserialized objects can indeed be annoying. When dealing with them, I find it's best to -match on the typename (which will work for both real and deserialized).

I think the bigger issue is my dependency on PowerShellHumanizer which would probably slow Posh's load down too much.

Eh, kinda/sorta / not really.

Since the .Length is already a -VirtualProperty, it wouldn't be too much difficulty to check to see if humanizer is loaded and then use the humanized version. Much like with how it treats TerminalIcons (if it's loaded, TerminalIcons will be preferred over Posh's icons).

P.S. I'll see if I can hook up the debugger and play with it too. I assume the issue is the process block deriving the table header based on the last object passed through.

Thanks for looking more into it. I'd love to figure out what the bug is here and try to include the fix in the current batch.

Fair warning on debugging: While the commands in EZOut are debuggable, .format.ps1xml files cannot be directly debugged. You can have a formatter that calls a command and break in there, but you can't put a breakpoint in a .format.ps1xml. The same holds true for .types.ps1xml. However, in either case, building the formatters and types in individual files will make them much easier to debug (since you can put a breakpoint in your scripts and run them directly with the same input).

@mattcargile
Copy link
Author

mattcargile commented Jul 15, 2024

Deserialized objects can indeed be annoying. When dealing with them, I find it's best to -match on the typename (which will work for both real and deserialized).

Right. I wanted to avoid the .pstypenames -match because I figured it would be slower than letting the engine handle it with the predefined xml. Maybe it is all a wash at the end because undoubtedly the formatter has to check the types somehow. I wanted my [scriptblock] to be simpler.

I think the bigger issue is my dependency on PowerShellHumanizer which would probably slow Posh's load down too much.

Eh, kinda/sorta / not really.

Since the .Length is already a -VirtualProperty, it wouldn't be too much difficulty to check to see if humanizer is loaded and then use the humanized version. Much like with how it treats TerminalIcons (if it's loaded, TerminalIcons will be preferred over Posh's icons).

I'd agree with that maybe I'll look at it after I figure out this issue.

P.S. I'll see if I can hook up the debugger and play with it too. I assume the issue is the process block deriving the table header based on the last object passed through.

Thanks for looking more into it. I'd love to figure out what the bug is here and try to include the fix in the current batch.

Fair warning on debugging: While the commands in EZOut are debuggable, .format.ps1xml files cannot be directly debugged. You can have a formatter that calls a command and break in there, but you can't put a breakpoint in a .format.ps1xml. The same holds true for .types.ps1xml. However, in either case, building the formatters and types in individual files will make them much easier to debug (since you can put a breakpoint in your scripts and run them directly with the same input).

So I did some debugging. I believe the issue is that $TableHeader is defined for each process{} iteration so the last object passed through will be retained. I'd think we would need to define $TableHeader in the begin{} block and then reconcile it on each iteration of the process{} block to make sure it works for all the column entires.

$TableHeader += "<TableColumnHeader>${Label}${alignment}${WidthTag}</TableColumnHeader>"

For instance, I think the $label should be defined on each iteration whether there is an $AliasProperty or $VirtualProperty.

It does seem fairly challenging to get right and the user has the opportunity to "break" it by not picking the same properties, as you probably know.

Additionally, shouldn't the below code have nm as the Label?

write-formatTableView -Property Name -Width 70  -ViewTypeName myview -AliasProperty @{ Name = 'nm' }

$label = ""
# If there was an alias defined for this property, use it
if ($AliasProperty.$p -or $VirtualProperty.$p) {
$label = "<Label>$p</Label>"
if ($VirtualProperty.$p) {
"<TableColumnItem><ScriptBlock>$([Security.SecurityElement]::Escape($VirtualProperty.$p))</ScriptBlock>$format</TableColumnItem>"
} else {
"<TableColumnItem><PropertyName>$($AliasProperty.$p)</PropertyName>$Format</TableColumnItem>"
}
} else {
"<TableColumnItem><PropertyName>$p</PropertyName>$Format</TableColumnItem>"
}
$TableHeader += "<TableColumnHeader>${Label}${alignment}${WidthTag}</TableColumnHeader>"

It feels like the AliasProperty logic should be reversed? Or do I need to write the code like the below?

write-formatTableView -Property nm -Width 70  -ViewTypeName myview -AliasProperty @{ nm = 'Name' }

@StartAutomating
Copy link
Owner

StartAutomating commented Sep 6, 2024

@mattcargile just checked in a fix for the pipeline behavior you observed. Please check out the branch or pull down the docker image and give it a try.

As far as the -AliasProperty and other related parameters, yes, you're meant to provide the code in the format about. The reasoning is that it you might want to alias multiple properties to the same underlying name.

-Property can be though of as the key to all of the -*Property dictionaries.

@mattcargile
Copy link
Author

mattcargile commented Sep 25, 2024

Still shows the [scriptblock] text as the header for Name.
image

There isn't a <Label> element below.
image

@mattcargile
Copy link
Author

Latest commits fixed it. Mode and Name show up as <Label>.

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

No branches or pull requests

2 participants