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

added support for ordered hashtables #17

Merged
merged 3 commits into from
Nov 20, 2019
Merged

added support for ordered hashtables #17

merged 3 commits into from
Nov 20, 2019

Conversation

jkrgr0
Copy link

@jkrgr0 jkrgr0 commented Oct 11, 2019

This fix allows the cmdlet New-MDTable to support ordered hashtables.
I've found out, that the cmdlet New-MDTable does not support ordered hashtables or [pscustomobject].
If i declare a [pscustomobject][ordered] with a defined order of properties, this order is ignored by the cmdlet and the column order of the table is randomly:

C:\> Import-Module MarkdownPS
C:\> $Object = [pscustomobject][ordered]@{
  Name = "ThisIsMyNameProperty"
  AnotherProperty = "This is another property"
  LastProperty = "This property should be the last column in the Markdown table"
}
C:\> New-MDTable -Object $Object
| AnotherProperty                                               | Name                                                          | LastProperty                                                  |
| ------------------------------------------------------------- | ------------------------------------------------------------- | ------------------------------------------------------------- | 
| This is another property                                      | ThisIsMyNameProperty                                          | This property should be the last column in the Markdown table |

The expected result of the cmdlet should be:

| Name                                                          | AnotherProperty                                               | LastProperty                                                  |
| ------------------------------------------------------------- | ------------------------------------------------------------- | ------------------------------------------------------------- |
| ThisIsMyNameProperty                                          | This is another property                                      | This property should be the last column in the Markdown table |

@Sarafian
Copy link
Owner

I've always had an issue with this part. I find your fix surprizingly easy and small :) but I would like to run my use cases with you to better understand.

I always did something like this adjusted to your example to achieve the same effect

    $columns=[ordered]@{
        Name =$null
        AnotherProperty =$null
        Scheme=$null
        LastProperty =$null
    }

There is an example about this in the main readme file

#region Tables
$markdown+=New-MDHeader "Tables"
$markdown+=New-MDParagraph -Lines "Without aligned columns"
$markdown+=Get-Command -Module MarkdownPS |Select-Object Name,CommandType,Version | New-MDTable
$markdown+=New-MDParagraph -Lines "With aligned columns"
$markdown+=Get-Command -Module MarkdownPS | New-MDTable -Columns ([ordered]@{Name="left";CommandType="center";Version="right"})
#endregion

The drivers for the -Columns property were:

  1. Override the order of columns upon rendering
  2. Implicitly filter out columns which could be also be done by a Select-Object

Notice that the -Columns is expected to be an ordered hash but it is not defined in the parameter and I don't remember why. When I chose the -Columns parameter as a solution, I had noticed that there was no connection between how objects would render in the output directly or through Format-Table with the properties. This is because PowerShell offers this concept of formatters for types that are not directly related to the type definitions.

So my question is which part of the problem does your solution address. My understanding is that this is about rendering object produced by the New-Object cmdlet like in your example. When developing did you try this with other inputs, like the examples included in the readme and mentioned above?

Can we add this pattern also in the tests

@jkrgr0
Copy link
Author

jkrgr0 commented Oct 21, 2019

To be honest, i've overlooked your example in the main readme file.

My solution is just a easy fix to not use the parameter -Columns explicitly. Additionally my expection is, if i provide a ordered hashtable to the cmdlet, that the cmdlet doesn't destroy the order of the ordered hashtable.

@jkrgr0
Copy link
Author

jkrgr0 commented Oct 21, 2019

I've now added new tests to verify if New-MDTable does not distord the order of the properties.

Additionally i've found a bug in the test -Object is null.
This test will always fail on non-english systems, because the string is different.
The test now validates against the FullyQualifiedErrorId of the error message. This should be the same on all systems.

@Sarafian
Copy link
Owner

Looks ok. But it is very busy so I'll take some time before publishing. Sorry. Btw, if you have some ideas, please do share or create issues.

@Sarafian Sarafian merged commit 4db8224 into Sarafian:master Nov 20, 2019
@Sarafian
Copy link
Owner

So I modified a bit the testing because it wasn't actually checking for the sequence. I added two randomized tests one for following the objects propery sequence and one to overwrite with specified columns. (In essence you can also render less column but a Select-Object can do that as well although I think this would mess up with the sequence.

Sorry for the delay I was away,.

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

Successfully merging this pull request may close these issues.

3 participants