Use parameter HelpMessage for tool tip completion #25108

jborean93 · 2025-02-28T03:59:20Z

PR Summary

Use the help message on the parameter in the completion tool tip to provide better completion help. This can be utilised by other tools to provide more context for a parameter outside of the current type and parameter name which is quite minimal.

PR Context

The current tool tip provided by MenuComplete for parameter is not too useful as it has just the parameter name and type. With this change it will also include the HelpMessage (or HelpMessageBaseName and HelpMessageResourceId on compiled cmdlets).

For example here is the output of Write-Progress today

Now this is the output with the changes in this PR.

I went with using HelpMessage for a few reasons:

It's available in the completion code so no expensive lookups
It can be used for dynamic parameters as they build the ParameterAttribute at runtime
The constraints around attribute field strings would mean it would most likely be a shortened description making it nice for the console to display

The final point I think is pretty important as CBH or MAML help could have quite long descriptions for a parameter which can be messy to display in the console. By having the HelpMessage set we can reserve the more detailed help for things like Get-Help to show but use this attribute field for things that display the tool tip (tip being short here).

PR Checklist

Use the help message on the parameter in the completion tool tip to provide better completion help. This can be utilised by other tools to provide more context for a parameter outside of the current type and parameter name which is quite minimal.

iSazonov · 2025-02-28T04:16:13Z

/cc @MartinGC94 Please review.

iSazonov

Looks great!

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

MartinGC94 · 2025-02-28T09:23:33Z

Looks good to me. My only concern was the tooltip showing the exception "Failed to find the help message: {e.Message}" but it seems like you already plan to change this.

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs

iSazonov · 2025-02-28T17:06:30Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs

        /// <returns></returns>
        private static List<CompletionResult> GetParameterCompletionResults(
            string parameterName,
            uint validParameterSetFlags,
            IEnumerable<MergedCompiledCommandParameter> parameters,
-            bool withColon)
+            bool withColon,
+            Assembly commandAssembly = null)


We don't but it saved me from specifying null for the argument where it is called. I can certainly make it without a default if you prefer that.

I do think the default is a nice way to see that the argument is nullable without having to turn on the nullability checks and annotating it.

I see your point. This is not critical, it just makes me think that there are a number of call sites that use this default.

In this case there still are, this is why I added the default in the first place because there are some calls where there is no assembly object to provide.

jborean93 · 2025-03-01T04:19:08Z

Thanks for the review @iSazonov, I've pushed a change that is a tiny bit different from your suggestion (still very similar). Please let me know if I've missed anything or if you want me to make the commandAssembly on GetParameterCompletionResults a non-default.

iSazonov · 2025-03-01T06:43:27Z

src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs

+                                if (helpMessage is not null && TryGetParameterHelpMessage(pattr, commandAssembly, out string attrHelpMessage))
+                                {
+                                    helpMessage = $" - {attrHelpMessage}";
+                                }


Last attribute win? In code above first win. I'd expect the same here.

It's somewhat confusing but the "first" attribute is actually the closes one to the definition rather than it being top -> bottom.

So in the below example the first attribute is actually Bottom, then Middle, then finally Top.

[Parameter(HelpMessage='Top')] [Parameter(HelpMessage='Middle')] [Parameter(HelpMessage='Bottom')] [string] $Param

This could change in the future but the code already goes bottom -> top and I don't think this PR should be the one to revisit that choice.

I mean line 791. There is a break but not here. Why is it different?

The code from 783-794 only check for the help message and does not check for DontShow (what it is doing today) so once we get a message we can cancel the loop entirely.

The code from 904-921 currently checks if DontShow is present hence why it breaks early there in that code branch. With this PR it also now checks for the help message if present but it doesn't break because it still needs to check for DontShow if present in subsequent attributes for the parameter.

I ask about follow difference:

In code from 783-794 we get help message from "Bottom" (first attribute in the loop)

In code from 904-921 we get help message from "Top" (last attribute in the loop)
Yes?

No differences they are both the same, both will loop through the attributes in the "compiled" order from PowerShell (bottom up). It will stop at the first attribute where there is a valid help message found.

The only reason why there is a difference in code is because one handles DontShow while the other does not. This is a behaviour not touched by this PR at all.

This could change in the future but the code already goes bottom -> top and I don't think this PR should be the one to revisit that choice.

Before this change, there was nothing in the result to distinguish one result from another for the same parameter. But now we have such property - Tooltip. That's why I can't agree with your statement. The results should be generated correct now.

In [string][int]$var first attribute win - [string]. The common expectation is that first parameter attribute win too and we see help message from the attribute.
Also if parameter has two parameter attributes for different parameter sets we should get help message from correct parameter set if possible or all cases.

I'm just going to have to leave this as is. This is beyond what I find needed for such a PR. Will just have to wait until someone from the pwsh team gets to this.

MSFT team has already clearly outlined the work that they will do in this milestone and they will not consider others as this exceeds their capabilities.

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

microsoft-github-policy-service · 2025-03-08T20:17:03Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

iSazonov · 2025-03-10T05:33:52Z

test/powershell/Host/TabCompletion/TabCompletion.Tests.ps1

+        if ($SingleMatch) {
+            $Script = 'Test-Function -ParamWithHelp'
+            $res = (TabExpansion2 -inputScript $Script).CompletionMatches
+        }
+        else {
+            $Script = 'Test-Function -'
+            $res = (TabExpansion2 -inputScript $Script).CompletionMatches | Where-Object CompletionText -eq '-ParamWithHelp'
+        }
+
+        $res.Count | Should -Be 1
+        $res.CompletionText | Should -BeExactly '-ParamWithHelp'
+        $res.ToolTip | Should -BeExactly $expected


It is better to check all the cases explicitly to avoid regression in any form (in tests below too):

Suggested change

if ($SingleMatch) {

$Script = 'Test-Function -ParamWithHelp'

$res = (TabExpansion2 -inputScript $Script).CompletionMatches

}

else {

$Script = 'Test-Function -'

$res = (TabExpansion2 -inputScript $Script).CompletionMatches | Where-Object CompletionText -eq '-ParamWithHelp'

}

$res.Count | Should -Be 1

$res.CompletionText | Should -BeExactly '-ParamWithHelp'

$res.ToolTip | Should -BeExactly $expected

if ($SingleMatch) {

$Script = 'Test-Function -ParamWithHelp'

$res = (TabExpansion2 -inputScript $Script).CompletionMatches

$res.Count | Should -Be 1

$res.CompletionText | Should -BeExactly '-ParamWithHelp'

$res.ToolTip | Should -BeExactly $expected

}

else {

$Script = 'Test-Function -'

$res = (TabExpansion2 -inputScript $Script).CompletionMatches | Where-Object CompletionText -eq '-ParamWithHelp'

$res.Count | Should -Be 2

$res[0].CompletionText | Should -BeExactly '-ParamWithHelp'

$res[0].ToolTip | Should -BeExactly $expected

$res[1].CompletionText | Should -BeExactly '-ParamWithoutHelp'

$res[1].ToolTip | Should -BeExactly '[Object] ParamWithoutHelp'

}

In this case we are only testing the ToolTip message for each returned option but even if you wanted to have multiple tests there are a few problems with this approach:

This will return all parameters including the common ones so adding more common parameters would mean you also need to update this test

I've already added a specific test for a parameter without the help message here https://github.com/PowerShell/PowerShell/pull/25108/files#diff-393e1fa171a673ad5e5c3c1f3700ad03d32d46031ffcf72e4eaac1a7c7c46579R1056-R1077

This may not apply to this change directly, but if it is possible to make an additional check, why not add it and exclude regression in case someone reworks this code?
My suggestion is to expand this test a bit and check that the text from the first parameter is not randomly displayed for the second one.

Because if attributes for another parameter are being applied to another we have a major problem that would/should be picked up by other tests. By testing it here as well we complicate an already somewhat complicated test that isn’t designed to check for this particular problem.

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Feb 28, 2025

Handle multiple Parameter attributes

2cae319

iSazonov reviewed Feb 28, 2025

View reviewed changes

Changes from review

662bce1

iSazonov reviewed Feb 28, 2025

View reviewed changes

JustinGrote mentioned this pull request Feb 28, 2025

Surface Additional Parameter Documentation/Tooltips PowerShell/PowerShellEditorServices#2222

Merged

1 task

Use TryGet pattern and simplify null checks

6b09e73

iSazonov reviewed Mar 1, 2025

View reviewed changes

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Mar 8, 2025

Update tests based on review comments

3a0952e

iSazonov reviewed Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use parameter HelpMessage for tool tip completion #25108

Use parameter HelpMessage for tool tip completion #25108

jborean93 commented Feb 28, 2025

iSazonov commented Feb 28, 2025

iSazonov left a comment

MartinGC94 commented Feb 28, 2025

iSazonov Feb 28, 2025

jborean93 Mar 1, 2025 •

edited

Loading

iSazonov Mar 1, 2025

jborean93 Mar 9, 2025

jborean93 commented Mar 1, 2025

iSazonov Mar 1, 2025

jborean93 Mar 9, 2025 •

edited

Loading

iSazonov Mar 10, 2025

jborean93 Mar 10, 2025

iSazonov Mar 10, 2025

jborean93 Mar 10, 2025 •

edited

Loading

iSazonov Mar 10, 2025

jborean93 Mar 10, 2025

iSazonov Mar 10, 2025

microsoft-github-policy-service bot commented Mar 8, 2025

iSazonov Mar 10, 2025

jborean93 Mar 10, 2025

iSazonov Mar 10, 2025

jborean93 Mar 10, 2025

Use parameter HelpMessage for tool tip completion #25108

Are you sure you want to change the base?

Use parameter HelpMessage for tool tip completion #25108

Conversation

jborean93 commented Feb 28, 2025

PR Summary

PR Context

PR Checklist

iSazonov commented Feb 28, 2025

iSazonov left a comment

Choose a reason for hiding this comment

MartinGC94 commented Feb 28, 2025

Choose a reason for hiding this comment

jborean93 Mar 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jborean93 commented Mar 1, 2025

Choose a reason for hiding this comment

jborean93 Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jborean93 Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

microsoft-github-policy-service bot commented Mar 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jborean93 Mar 1, 2025 •

edited

Loading

jborean93 Mar 9, 2025 •

edited

Loading

jborean93 Mar 10, 2025 •

edited

Loading