Show optional parameters as such when dislplaying method definition and overloads by eugenesmlv · Pull Request #13799 · PowerShell/PowerShell

eugenesmlv · 2020-10-17T16:03:23Z

PR Summary

These changes provide more informative method definition for methods with the optional parameters.

Primitive value types and strings.

static void Bar(int optParam = -1)
static void Bar(string optParam = "default string")

Reference types.

static void Bar(string optParam = null)

Enums.

static void Bar(System.UriKind optParam = System.UriKind.Relative)

Structs and generic method parameters.

static void Bar(datetime optParam = default)
static void GenericBar[T](T optParam = default)

PR Context

Fix #13728

The changes don't cover a case with an OptionalAttribute. Calling such a method without optional arguments throws an exception.

Add-Type -NameSpace demo -Name Foo -MemberDefinition @"
public static void Bar([System.Runtime.InteropServices.Optional]int optParam) { }
"@

[demo.Foo]::Bar()
#OperationStopped: Expression of type 'System.Reflection.Missing' cannot be used for parameter of type 'System.Int32' of method 'Void Bar(Int32)' (Parameter 'arg0')

I could investigate it if you show me where to start.

PR Checklist

…as such (PowerShell#13728)

eugenesmlv · 2020-10-17T16:50:40Z

@iSazonov it seems like CompletionCompleters also uses a method definition for the tooltip. Is it possible to test that case somehow?

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

Lines 5312 to 5318 in 67bdf72

    
           var methodCacheEntry = member as DotNetAdapter.MethodCacheEntry; 
        
           if (methodCacheEntry != null) 
        
           { 
        
               memberName = methodCacheEntry[0].method.Name; 
        
               isMethod = true; 
        
               getToolTip = () => string.Join("\n", methodCacheEntry.methodInformationStructures.Select(m => m.methodDefinition)); 
        
           }

iSazonov

it seems like CompletionCompleters also uses a method definition for the tooltip. Is it possible to test that case somehow?

You can use TabCompletion2 (see our tests how it is used).

eugenesmlv · 2020-10-19T11:22:15Z

You can use TabCompletion2 (see our tests how it is used).

I couldn't find TabCompletion2 anywhere. Do you mean TabExpansion2? Anyway, as far as I understand, tab completion completes just method name and there are already tests for that.

Do you mean "method signature"?

Rather, "method signature with parameters of various types such as parameters passed by reference, generic and optional parameters", but it seems to me, we could combine tests for the overloaded definition and different method signature into one context.

I also want to suggest to mark parameters passed by reference with [in] and [out] keywords in addition to the [ref]. Can I implement that in this PR?

SeeminglyScience · 2020-10-19T14:21:23Z

You can use TabCompletion2 (see our tests how it is used).

I couldn't find TabCompletion2 anywhere. Do you mean TabExpansion2? Anyway, as far as I understand, tab completion completes just method name and there are already tests for that.

Tab expansion results include a tooltip, which is only shown in editors and some PSReadLine key handlers (but not the Tab one). You can see it here:

(TabExpansion2 -inputScript ($s = '$Host.PushRunspace') -cursorColumn $s.Length).CompletionMatches

returns

CompletionText ListItemText ResultType ToolTip
-------------- ------------ ---------- -------
PushRunspace(  PushRunspace     Method void PushRunspace(runspace runspace)…

I also want to suggest to mark parameters passed by reference with [in] and [out] keywords in addition to the [ref]. Can I implement that in this PR?

Maybe best to open an issue to discuss that one. Part of the problem there is that [in] and [out] aren't really a thing in PowerShell, so the way to pass it will still be [ref].

iSazonov · 2020-10-19T15:12:50Z

I couldn't find TabCompletion2 anywhere. Do you mean TabExpansion2?

Yes, sorry.

eugenesmlv · 2020-10-19T18:28:25Z

@iSazonov @SeeminglyScience thanks for helping me! I've added tests for the TabExpansion2 tooltip.

ghost · 2020-10-27T14:00:12Z

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

ghost · 2021-09-23T02:00:42Z

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

SteveL-MSFT

LGTM

iSazonov · 2022-05-18T05:13:24Z

/rebase

github-actions · 2022-05-18T05:14:01Z

Started rebase: https://github.com/PowerShell/PowerShell/actions/runs/2343284398

GitHub
Show optional parameters as such when dislplaying method definition and overloads · PowerShell/PowerShell@3f35800
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

iSazonov · 2022-05-18T05:16:23Z

@TravisEz13 Could you please look the rebase error? https://github.com/PowerShell/PowerShell/runs/6483334261?check_suite_focus=true#step:5:31

GitHub
Show optional parameters as such when dislplaying method definition and overloads · PowerShell/PowerShell@3f35800
PowerShell for every system! Contribute to PowerShell/PowerShell development by creating an account on GitHub.

ghost · 2022-05-25T14:01: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

ghost · 2022-07-18T21:42:52Z

🎉v7.3.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

Extend GetMethodInfoOverloadDefinition to show optional parameters …

923a747

…as such (PowerShell#13728)

ghost assigned TravisEz13 Oct 17, 2020

Fix CodeFactor style issue

06cf7e7

eugenesmlv changed the title ~~Show optional parameters as such wheh dislplaying method definition and overloads~~ Show optional parameters as such when dislplaying method definition and overloads Oct 17, 2020

iSazonov reviewed Oct 19, 2020

View reviewed changes

iSazonov added CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Hacktoberfest-Accepted Accepted to participate in Hacktoberfest labels Oct 19, 2020

Fix review issues

100673b

Add tests for TabExpansion2 ToolTip

440f0f6

Fix tooltip string splitting

2b8c2a5

iSazonov reviewed Oct 20, 2020

View reviewed changes

Comment thread test/powershell/engine/Api/GetMethodInfoOverloadDefinition.Tests.ps1 Outdated

iSazonov reviewed Oct 20, 2020

View reviewed changes

Comment thread test/powershell/engine/Api/GetMethodInfoOverloadDefinition.Tests.ps1 Outdated

eugenesmlv added 2 commits October 20, 2020 13:07

Fix tests

600edf1

Bring back the namespace

a962c82

iSazonov approved these changes Oct 20, 2020

View reviewed changes

iSazonov requested a review from SteveL-MSFT October 20, 2020 13:20

ghost added the Review - Needed The PR is being reviewed label Oct 27, 2020

SteveL-MSFT added the Backport-7.2.x-Consider label Dec 15, 2020

adityapatwardhan removed the Backport-7.2.x-Consider label Sep 15, 2021

ghost removed the Review - Needed The PR is being reviewed label Sep 15, 2021

ghost added the Review - Needed The PR is being reviewed label Sep 23, 2021

SteveL-MSFT approved these changes May 16, 2022

View reviewed changes

SteveL-MSFT requested a review from daxian-dbw May 16, 2022 23:22

ghost removed the Review - Needed The PR is being reviewed label May 18, 2022

ghost added the Review - Needed The PR is being reviewed label May 25, 2022

TravisEz13 approved these changes Jun 27, 2022

View reviewed changes

TravisEz13 merged commit 735096b into PowerShell:master Jun 27, 2022

ghost removed the Review - Needed The PR is being reviewed label Jun 27, 2022

ghost mentioned this pull request Jul 18, 2022

When displaying method overloads, show optional parameters as such #13728

Closed

TravisEz13 mentioned this pull request Sep 30, 2022

Move ApiScan to compliance build #18191

Merged

22 tasks

Conversation

eugenesmlv commented Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

eugenesmlv commented Oct 17, 2020

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eugenesmlv commented Oct 19, 2020

Uh oh!

SeeminglyScience commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Oct 19, 2020

Uh oh!

eugenesmlv commented Oct 19, 2020

Uh oh!

Uh oh!

Uh oh!

ghost commented Oct 27, 2020

Uh oh!

ghost commented Sep 23, 2021

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov commented May 18, 2022

Uh oh!

github-actions Bot commented May 18, 2022 • edited by unfurl-links Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented May 18, 2022 • edited by unfurl-links Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 25, 2022

Uh oh!

ghost commented Jul 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

eugenesmlv commented Oct 17, 2020 •

edited

Loading

SeeminglyScience commented Oct 19, 2020 •

edited

Loading

github-actions Bot commented May 18, 2022 •

edited by unfurl-links Bot

Loading

iSazonov commented May 18, 2022 •

edited by unfurl-links Bot

Loading