-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Fix positional argument completion #17796
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
Fix positional argument completion #17796
Conversation
$Param2 | ||
) | ||
} | ||
Verb-Noun -Param1 Hello ^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about more complex test like (expect Value3 in parameter position 3)
Verb-Noun -ParamPos0 Value0 ParamPos1Value1 -ParamPos2 Value2 ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary? The logic would be the same as in the previous test: It takes the next available positional parameter. It doesn't really matter if it's the first, third or even the 30th parameter position it's looking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is too big to observe its logic in its entirety, and the test I'm asking about is a case I thought it might not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. Here's how it works: The target position is found elsewhere and is based on how many positionally bound parameters have been specified, in the above example it would look for the second positional parameter (position 1) because one other positional parameter has been found.
The loop I've modified loops over the unbound parameters and parametersets that are valid based on the currently bound parameters, here it looks for any positional parameters with a position greater than or equal to the target location. It then saves whichever parameter has the lowest position and binds the argument to that.
Because it's looping over unbound parameters all the previous parameters "ParamPos0", "ParamPos1" and "ParamPos2" are automatically filtered out so the next available parameter would be "ParamPos3".
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
…letionCompleters.cs Co-authored-by: Ilya <darpa@yandex.ru>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@MartinGC94 Can you please rebase? I think the failures come from a different PR that was merged. |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
@MartinGC94 Thanks for the improvements! |
🎉 Handy links: |
PR Summary
Fixes #17653
Instead of looking for a parameter with the exact location specified, it instead looks for the parameter with a position closest to the specified location. This fixes completion for positional parameters where an earlier positional parameter has been specified by name so the completion should look for the next positional parameter.
PR Context
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).