PowerShell / platyPS

Write PowerShell External Help in Markdown
MIT License
767 stars 149 forks source link

Positional Parameters use 0-based positioning instead of 1-based positioning in generated markdown files #440

Closed fourpastmidnight closed 3 years ago

fourpastmidnight commented 5 years ago

Steps to reproduce

  1. Create a PowerShell cmdlet with at least two positional parameters
    1. According to about_Functions_Advanced_Parameters, a "position value of '0' represents the first position in a command..."
  2. Issue a command for platyPS to either generate a new Markdown help file or update an existing Markdown help file. (I.e. both New-MarkdownHelp and Update-MarkdownHelp are affected.)

Expected behavior

I expected the YAML generated for parameter metadata would result in 1-based positioning for positional parameters in keeping in line with how Microsoft displays parameter help in its own PowerShell cmdlets.

Actual behavior

The generated parameter YAML metadata is using 0-based positioning for positional parameters. Mind you, I haven't actually generated the Help file yet—so I'm unsure as to whether or not the positions are "adjusted" during that creation process. However, please see below under Environment Data for more information regarding this current behavior.

Environment data

I'm using PSDepend to keep my modules up-to-date with those published to the PowerShell Gallery on every build of my module. The latest installed version on my machine is 0.12.0.

Now, running Get-Module -ListAvailable platyPS reveals that I also have v. 0.11.0 installed. Furthermore, looking at a project I haven't touched in a while, for which I also generated help using platyPS (presumably the 0.11.0 version), the YAML output for positional parameters was 1-based for those generated makdown help files.

fourpastmidnight commented 5 years ago

Looking at the schema document for v2.0 of the schema, this behavior also seems to contradict the schema which indicates that the generated Position attribute will be {1..n} | Named.

vors commented 5 years ago

Oh good call! That should be an easy fix. Would you consider sending a pull request for it? I'd be happy to help you navigate the code, if you have any questions.

fourpastmidnight commented 5 years ago

Sure thing.

Sent from my Windows 10 phone

From: Sergei Vorobev Sent: Sunday, February 10, 2019 12:49 To: PowerShell/platyPS Cc: Craig E. Shea; Author Subject: Re: [PowerShell/platyPS] Positional Parameters use 0-basedpositioning instead of 1-based positioning in generated markdown files (#440)

Oh good call! That should be an easy fix. Would you consider sending a pull request for it? I'd be happy to help you navigate the code, if you have any questions. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

fourpastmidnight commented 5 years ago

Lol, I thought this would be "easy". But I must admit, I'm having a hard time finding my way around the codebase. I'll keep looking, but any pointers would be appreciated. 😉

fourpastmidnight commented 5 years ago

I think I found my way around. 😄

vors commented 5 years ago

Alright!

vors commented 5 years ago

If you haven't seen already https://github.com/PowerShell/platyPS/blob/master/CONTRIBUTING.md#contributing-to-platyps has some good starting info about the code layout.

fourpastmidnight commented 5 years ago

I'm beginning to think this is only an issue if the cmdlet does not have comment-based help when documentation is generated. Not sure, need to do more tests to confirm.

fourpastmidnight commented 5 years ago

No, platyPs is not handling numbering positional parameters correctly in other instances. I've modified existing tests and added additional assertions which make the tests fail. Now I just need to figure out how to make them pass. 😉

vors commented 5 years ago

TDD! Love that.

fourpastmidnight commented 5 years ago

OK, so, I was wrong about platyPS not doing the right thing in other places. I made a change to the code and forgot about it. Anyway, long story short, there is an issue, but I don't think it resides with platyPS, but resides with PowerShell itself.

In test/Pester/PlatyPs.Tests.ps1, there are two contexts under the describe New-MarkdownHelp:

Under both of these contexts, I updated the test function parameters as follows:

function Test-PlatyPsFunction {
    param (
        [Switch]$Common,
        [Parameter(Position = 0, ParameterSetName = 'First', HelpMessage = 'First parameter help description')]
        [string]$First
        # Changed parameter type because in the real world, PS wouldn't be able to 
        # differentiate the parameter sets and didn't know if maybe this was causing the issue...
        [Parameter(Position = 0, ParameterSetName = 'Second')]
        [int]$Second,
        [Parameter(Position = 1)]
        [int]$Third
    )

    Write-Host "`$Common: $Common"
    Write-Host "`$First:  $First"
    Write-Host "`$Second: $Second"
    Write-Host "`$Third:  $Third"
}

I then modified and added the following It tests for both contexts:

It 'generates markdown with correct parameter set names' {
    ($content | Where-Object {$_ -eq 'Parameter Sets: (All)'} | Measure-Object).Count | Should Be 2
    ($content | Where-Object {$_ -eq 'Parameter Sets: First'} | Measure-Object).Count | Should Be 1
    ($content | Where-Object {$_ -eq 'Parameter Sets: Second'} | Measure-Object).Count | Should Be 1
}

It 'generates markdown with correct parameter positions' {
    ($content | Where-Object {$_ -eq 'Position: 0'} | Measure-Object).Count | Should be 0
    ($content | Where-Object {$_ -eq 'Position: 1'} | Measure-Object).Count | Should Be 2
    ($content | Where-Object {$_ -eq 'Position: 2'} | Measure-Object).Count | Should Be 1
    ($content | Where-Object {$_ -eq 'Position: 3'} | Measure-Object).Count | Should Be 0
}

The tests executed exactly as I thought they would, though, I still didn't know why. They all passed for the context Generated markdown features: comment-based help. But the second It test shown above failed only for the context Generated markdown features: no comment-based help.

By this time, I've stepped through the platyPs code many times and have become familiar with how it's generating PowerShell help. Basically, the tests use Get-Help to generate, in this case, an ExtendedCmdletHelpInfo object, which is then used to generate a Maml document object graph, which is then transformed to markdown. So I decided to start with Get-Help since this is where it all begins. And this is where it gets interesting.

$testFnWithHelp = {
  <#
    .SYNOPSIS
    A function with comment-based help

    .DESCRIPTION
    This function has comment-based help. Its help, however, does not cover all
    of the function parameters. But, it still results in correct auto-generated help
    by PowerShell.

    .PARAMETER Second
    This is the only documented parameter

    .EXAMPLE
    PS C:\> & $testFn 2
    $Common: False
    $First:
    $Second: 2
    $Third:  0

  #>
  param (
    [Switch]$Common,
    [Parameter(Position = 0, ParameterSetName = 'First', HelpMessage = 'First parameter help description')]
    [string]$First,
    [Parameter(Position = 0, ParameterSetName = 'Second')]
    [int]$Second,
    [Parameter(Position = 1)]
    [int]$Third
  )
  Write-Host "`$Common: $Common"
  Write-Host "`$First:  $First"
  Write-Host "`$Second: $Second"
  Write-Host "`$Third:  $Third"
}

$testFnWithNoHelp = {
  param (
    [Switch]$Common,
    [Parameter(Position = 0, ParameterSetName = 'First', HelpMessage = 'First parameter help description')]
    [string]$First,
    [Parameter(Position = 0, ParameterSetName = 'Second')]
    [int]$Second,
    [Parameter(Position = 1)]
    [int]$Third
  )
  Write-Host "`$Common: $Common"
  Write-Host "`$First:  $First"
  Write-Host "`$Second: $Second"
  Write-Host "`$Third:  $Third"
}

New-Item -Path Function:\TestFnWithHelp -Value $testFnWithHelp
New-Item -Path Function:\TestFnWithNoHelp -Value $testFnWithNoHelp

Write-Host "No comment-based help parameter positions:"
(Get-Help -Full TestFnWithNoHelp).Parameters.parameter |
Select-Object -Property Name,Position |
Format-Table

Write-Host "-"*60

Write-Host "Comment-based help parameter positions:"
(Get-Help -Full TestFnWithHelp).Parameters.parameter |
Select-Object -Property Name,Position |
Format-Table

Remove-Item Function:\TestFnWithHelp
Remove-Item Function:\TestFnWithNoHelp
Remove-Variable testFnWithHelp
Remove-Variable testFnWithNoHelp

Running the code above results in the following output:

No comment-based help parameter positions:

name   position
----   --------
Common Named   
First  0       
Second 0       
Third  1       

------------------------------------------------------------
Comment-based help parameter positions:

name   position
----   --------
Common named   
First  1       
Second 1       
Third  2       

Therefore, this is not a bug in platyPS.

fourpastmidnight commented 5 years ago

I did two more tests just to fully flesh out my understanding. I was pretty sure I knew what the results would be, but I had to try. Here's the code and output:

$testFnWithNoHelp2 = {
  param (
    [Switch]$Common,
    [Parameter(Position = 1, ParameterSetName = 'First', HelpMessage = 'First parameter help description')]
    [string]$First,
    [Parameter(Position = 1, ParameterSetName = 'Second')]
    [int]$Second,
    [Parameter(Position = 2)]
    [int]$Third
  )
}

$testFnWithHelp2 = {
  <#
  .SYNOPSIS
  This much help is enough to affect the outcome.
  #>

  param (
    [Switch]$Common,
    [Parameter(Position = 1, ParameterSetName = 'First', HelpMessage = 'First parameter help description')]
    [string]$First,
    [Parameter(Position = 1, ParameterSetName = 'Second')]
    [int]$Second,
    [Parameter(Position = 2)]
    [int]$Third
  )
}

New-Item -Path Function:\TestFnWithHelp2 -Value $testFnWithHelp2
New-Item -Path Function:\TestFnWithNoHelp2 -Value $testFnWithNoHelp2

Write-Host "No comment-based help parameter positions:"
(Get-Help -Full TestFnWithNoHelp2).Parameters.parameter |
Select-Object -Property Name,Position |
Format-Table

Write-Host "-"*60

Write-Host "Comment-based help parameter positions:"
(Get-Help -Full TestFnWithHelp2).Parameters.parameter |
Select-Object -Property Name,Position |
Format-Table

Remove-Item Function:\TestFnWithHelp2
Remove-Item Function:\TestFnWithNoHelp2
Remove-Variable testFnWithHelp2
Remove-Variable testFnWithNoHelp2

And here are the results:

Comment-based help parameter positions:

name   position
----   --------
Common Named   
First  1       
Second 1       
Third  2       

Comment-based help parameter positions:

name   position
----   --------
Common named   
First  2       
Second 2       
Third  3

So, any comment-based help is enough to affect how the PowerShell help system indexes function parameter positions for help "generated on the fly".

vors commented 5 years ago

Thank you digging into that! I think it's worth to open an issue in https://github.com/powershell/powershell about this inconsistency.

Sometimes we workaround such problems in PlatyPS, I'm unsure is it worth doing in this case. If you are interested in pushing this forward, I'd be happy to merge a workaround fix.

fourpastmidnight commented 5 years ago

I tried working around it, but I think you’re right: We’re likely to cause more problems than solve by attempting to work around it.

Sent from my Windows 10 phone

From: Sergei Vorobev Sent: Saturday, February 23, 2019 12:45 To: PowerShell/platyPS Cc: Craig E. Shea; Author Subject: Re: [PowerShell/platyPS] Positional Parameters use 0-basedpositioning instead of 1-based positioning in generated markdown files (#440)

Thank you digging into that! I think it's worth to open an issue in https://github.com/powershell/powershell about this inconsistency. Sometimes we workaround such problems in PlatyPS, I'm unsure is it worth doing in this case. If you are interested in pushing this forward, I'd be happy to merge a workaround fix. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

sdwheeler commented 3 years ago

In the current version 0.14.1, parameters are still 0-based. This is by design and preferred. Changing the documentation to be 1-based will cause confusion because PowerShell (Get-Command) shows a 0-based value. We should not make the documentation be different.