Sitelet https://web.archive.org/web/20211215095814/https://github.com/PowerShell/PowerShell/pull/13799
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

Jump to bottom

Show optional parameters as such when dislplaying method definition and overloads #13799

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Show optional parameters as such when dislplaying method definition and overloads #13799

wants to merge 7 commits into from
+212 −0

Conversation

@eugenesmlv
Copy link
Contributor

@eugenesmlv eugenesmlv commented Oct 17, 2020 •
edited
Loading

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
@eugenesmlv
Copy link
Contributor Author

@eugenesmlv eugenesmlv commented Oct 17, 2020

@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));
}

Loading

@eugenesmlv 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
iSazonov reviewed Oct 19, 2020
View changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

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).

Loading

src/System.Management.Automation/engine/CoreAdapter.cs Outdated Show resolved Hide resolved
Loading
test/powershell/engine/Api/GetMethodInfoOverloadDefinition.Tests.ps1 Outdated Show resolved Hide resolved
Loading
test/powershell/engine/Api/GetMethodInfoOverloadDefinition.Tests.ps1 Outdated Show resolved Hide resolved
Loading
test/powershell/engine/Api/GetMethodInfoOverloadDefinition.Tests.ps1 Outdated Show resolved Hide resolved
Loading
test/powershell/engine/Api/GetMethodInfoOverloadDefinition.Tests.ps1 Outdated Show resolved Hide resolved
Loading
@eugenesmlv
Copy link
Contributor Author

@eugenesmlv eugenesmlv commented Oct 19, 2020

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?

Loading

@SeeminglyScience
Copy link
Contributor

@SeeminglyScience SeeminglyScience commented Oct 19, 2020 •
edited
Loading

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].

Loading

@iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Oct 19, 2020

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

Yes, sorry.

Loading

@eugenesmlv
Copy link
Contributor Author

@eugenesmlv eugenesmlv commented Oct 19, 2020

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

Loading

@iSazonov iSazonov requested a review from SteveL-MSFT Oct 20, 2020
@msftbot
Copy link

@msftbot msftbot bot commented Oct 27, 2020

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

Loading

@msftbot msftbot bot removed the Review - Needed label Sep 15, 2021
@msftbot
Copy link

@msftbot msftbot bot commented Sep 23, 2021

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iSazonov iSazonov

@SteveL-MSFT SteveL-MSFT

Assignees

@TravisEz13 TravisEz13

Labels
CL-General Hacktoberfest-Accepted Review - Needed
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

When displaying method overloads, show optional parameters as such
6 participants
@eugenesmlv @SeeminglyScience @iSazonov @TravisEz13 @SteveL-MSFT @adityapatwardhan