platyPS icon indicating copy to clipboard operation
platyPS copied to clipboard

Missing <command:syntaxItem> in the output maml file

Open f0rt opened this issue 7 years ago • 22 comments

I have a markdown help file ( Get-Test.md.txt ) created with New-MarkdownHelp that has 2 parameter sets. Calling New-ExternalHelp results in a MAML that has only 1 <command:syntaxItem>. After a little effort to isolate it I found that it is only reproduced when all parameter definitions in the .md file mention the second parameter sets or (All). If I change a parameter set name from (All) to "Default" the MAML file is generated correctly and contains 2 <command:syntaxItem>.


PlatyPS version 0.11.1

f0rt avatar Oct 01 '18 11:10 f0rt

I tried it on the attached md file and it generates 2 param sets as expected. Can you attach the problematic markdown?

when all parameter definitions in the .md file mention the second parameter sets or (All).

So if I try

function foo
{
    param(
        [Parameter(ParameterSetName="p1")]
        [string]$foo,

        [string]$bar
    )
}

Then Get-Help puts it all into a single parameter set.

PS /Users/vors/Downloads> Get-Help foo

NAME
    foo
    
SYNTAX
    foo [-foo <string>] [-bar <string>] [<CommonParameters>]
    

ALIASES
    None
    

REMARKS
    None

vors avatar Oct 02 '18 03:10 vors

Sorry. I've uploaded wrong file. Get-Test.md.txt So the file uploaded above results in only 1 syntax item with 1 required parameter and 1 optional parameter which is not correct. It should generate additional syntax item with 1 optional parameter.

f0rt avatar Oct 02 '18 07:10 f0rt

Did you modify the markdown parameter sets or syntax? It seems that if you have a single parameter set declared on 1 parameter and no parameter set on another parameter, powershell will just assume you have a single parameter set.

You can see that for yourself:

function foo
{
    param(
        [Parameter(ParameterSetName="p1")]
        [string]$foo,

        [string]$bar
    )
}
PS /Users/vors/Downloads> (Get-Command foo).ParameterSets | ft      

Name IsDefault Parameters
---- --------- ----------
p1       False {foo, bar, Verbose, Debug...}

vors avatar Oct 03 '18 04:10 vors

The problem with the single parameter set(as showed in the example md) is that the default parameter set may have only optional parameters and the second parameter set may have a required parameter. So the generated (single) syntax item now has a mandatory parameter which means the parameter is always mandatory. This is not correct since the default parameter set has no mandatory parameters. This is not reproduced with the UI help editor.

I've generated the md file by using New-MarkDownHelp from binary module (c#). Since the problem exists with a relatively big cmdlet I've isolated the issue by removing and renaming some parts of the MD.

f0rt avatar Oct 03 '18 12:10 f0rt

Oh I see your point. Looks like powershell doesn't handle this case separately:

function foo
{
    param(
        [Parameter(ParameterSetName="p1", Mandatory=$true)]
        [string]$foo,
        [Parameter(Mandatory=$false)]
        [string]$bar
    )
}
PS /Users/vors/Downloads> foo -bar 123

cmdlet foo at command pipeline position 1
Supply values for the following parameters:
foo: 
PS /Users/vors/Downloads> (Get-Command foo).ParameterSets | ft     
    
Name IsDefault Parameters
---- --------- ----------                                      
p1       False {foo, bar, Verbose, Debug...}

vors avatar Oct 07 '18 22:10 vors

I see that PS has a problem too. I'm not sure how this is related to MD to MAML conversion since the markdown already contains 2 syntax items and the result MAML file contains only 1 syntax item.

f0rt avatar Oct 08 '18 08:10 f0rt

See my latest example that illustrates the problem correctly. The cmdlet has 2 parameter sets but the "Default" parameter set syntax item is not generated. Get-Test.md.txt

f0rt avatar Oct 08 '18 11:10 f0rt

I've modified your example to create a repro script:

function foo
{
    param(
        [Parameter(ParameterSetName="Default")]
        [Parameter(ParameterSetName="p1")]
        [string]$bar,
        [Parameter(ParameterSetName="p1", Mandatory=$true)]
        [string]$foo

    )
}

New-MarkdownHelp -Command foo -OutputFolder ./
New-ExternalHelp -Path .\foo.md -OutputPath ./

Note the result MAML file contains 1 syntax item with 2 paramters. The default parameter set syntax item is missing.

f0rt avatar Oct 08 '18 12:10 f0rt

Ah I finally see what you mean :) Note that Default is not special name for that matter, it will work with any other name.

vors avatar Oct 11 '18 03:10 vors

Can you please take a look at the PR above that contains the fix?

f0rt avatar Oct 12 '18 13:10 f0rt

oh forgot to click "publish the review" button

vors avatar Oct 13 '18 04:10 vors

Oh, I got it all wrong. :) I now see the problem in the code. Let me summarize:

Problem: The "GatherSyntax()" doesn't have enough information to generate the correct syntax items since it relies only on information in the "## PARAMETERS" section. This section may not contain all parameter set names because if parameter is in all parameter sets it is marked as "Parameter Sets: (All)".

Possible solutions:

  • Option 1 (Recommended): Generate MAML syntax items based on the information in the MD. Is there a reason the code is not implemented that way?
  • Option 2: Collect information about parameter sets from "parameters" + "syntax" sections in the MD file. Use it in GatherSyntax() to generate correctly syntax items.
  • Option 3: Drop "(All)" moniker and expand to all parameter set names when generating the MD file. This will eliminate any possible problems. Example: "Parameter Sets: (All)" -> "Parameter Sets: ParameterSet1, ParameterSet2".

Please share thoughts on the Options listed above. Thanks, f0rt

f0rt avatar Oct 16 '18 10:10 f0rt

Ah that's right!

We cannot drop (All) because this is what PowerShell uses when there are no parameterSets specified. I.e.

function foo
{
    param(
        [Parameter(ParameterSetName="Default")]
        [Parameter(ParameterSetName="p1")]
        [string]$bar,
        [Parameter(ParameterSetName="p1", Mandatory=$true)]
        [string]$foo,
        [string]$alll
    )
}

Get-Help foo would give us

PARAMETERS
    -alll <string>
        
        Required?                    false
        Position?                    Named
        Accept pipeline input?       false
        Parameter set name           (All)
        Aliases                      None
        Dynamic?                     false
        
    -bar <string>
        
        Required?                    false
        Position?                    Named
        Accept pipeline input?       false
        Parameter set name           p1, Default
        Aliases                      None
        Dynamic?                     false
        
    -foo <string>
        
        Required?                    true
        Position?                    Named
        Accept pipeline input?       false
        Parameter set name           p1
        Aliases                      None
        Dynamic?                     false

We should distinguish (All) that arises when there no parameter sets specified from the full enumeration of all parameter sets.

vors avatar Oct 17 '18 03:10 vors

What about Option1?

f0rt avatar Oct 17 '18 07:10 f0rt

Syntax section is currently write-only and that's how it's designed to be. This is done to avoid dealing with problems when it's a) misformatted b) inconsistent with parameters metadata section

vors avatar Oct 20 '18 03:10 vors

So we're going with Option 2? If not please propose solution and I'll implement. Thanks.

f0rt avatar Oct 22 '18 07:10 f0rt

Wait, I'm confused. What's the difference between 1 and 2? I was proposing to do a variant of 3, where we distinguish cases when we have implicit (All) when any parameter names are omitted and an explicit enumeration of all param sets. The later one should be expanded as suggested, the former should be kept as is.

vors avatar Oct 24 '18 06:10 vors

  • In Option 1 the syntax items are copied directly from SYNTAX section in the MD file. In Option 2 the syntax items are generated again based on the parameter set names in the SYNTAX section and the information in the PARAMETERS section. I guess we are dropping these since the SYNTAX section is "write-only".

I was proposing to do a variant of 3, where we distinguish cases when we have implicit (All) when any parameter names are omitted and an explicit enumeration of all param sets. The later one should be expanded as suggested, the former should be kept as is.

This will work only if the users create/update the MD file with the PlatyPS cmdlets. If they do it manually it will be a little confusing. I guess should be documented somehow (comment section in the MD file etc.). Anyway, they solution sounds good enough for me. Let me know if you are still ok with it and I'll start implementing it.

f0rt avatar Oct 24 '18 07:10 f0rt

If they do it manually

Parameters metadata, in general, should not be touched manually. It could in theory, but Update-MarkdownHelp should be doing a much better job for free.

Yes, that plan sounds good to me. Thank you for taking a stab at it!

vors avatar Oct 26 '18 04:10 vors

Hi!

I have replaced "(All)" with ParameterSets names separated by comma. (I fixed it in Markdownv2Renderer.cs, Method : AddParameter) Only two unit tests failed because of the expected result (They expected "(All)"), The names of unit tests which failed are: RenderesAllParameterSetMoniker and RendererProduceMarkdownV2Output I fixed them to expect ParameterSets names.

I have two questions:

Is it OK to be fixed that way? Do you need "(All)" for some specific case which is not covered by Unit tests?

I would like to add some additional unit tests. Are you ok?

Best Regards, Ivanka Zaycheva

vzay avatar Dec 04 '18 09:12 vzay

Hi @vzay ! Yes, thats the right way to proceed, but we should only unroll the (All) into the explicit list of all parameter names when it's that way in the source code. That would be consistent with how default generated help works (see https://github.com/PowerShell/platyPS/issues/401#issuecomment-430472533) Basically there is a logic somewhere in platyPS that checks that we cover all parameter cases and turns them into (All) and we should delete it.

vors avatar Dec 05 '18 07:12 vors

+1 - Azure PowerShell team is running into this issue as well. Let me know what I can do to push this fix along :)

maddieclayton avatar Feb 28 '19 23:02 maddieclayton

Based on review of the conversation, it appears that this is by design. Wait for Microsoft.PowerShell.PlatyPS v1 and see if you think there needs to be a change in what it produces.

sdwheeler avatar May 31 '24 16:05 sdwheeler