-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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
Improve hashtable completion #16498
Improve hashtable completion #16498
Conversation
…ady in a hashtable and add argument completers for Get-WinEvent and Invoke-CimMethod
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
…ameter for Cim cmdlets.
src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
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 with one minor comment.
src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs
Outdated
Show resolved
Hide resolved
@daxian-dbw can you have a look? |
Co-authored-by: Aditya Patwardhan <adityap@microsoft.com>
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
I understand if you are busy, to help prioritize I will say that out of all my open PRs, this is the one I'm most interested in seeing in the near future. It's just so convenient being able to tab complete parameters in splatting hashtables. |
@MartinGC94 Sorry for the long delay on this. I will get to this PR this week. |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
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.
Sorry for the delay on review.
The changes overall look good. Thanks for the improvements!
src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionAnalysis.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Dongbo Wang <dongbow@microsoft.com>
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
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) |
The |
🎉 Handy links: |
Sorry if this is not the right place to comment but I've just noticed this a few days ago and I must say it's amazing. Thank you for the hard work! 💪 |
@MartinGC94 don't know if you have a presence in twitter or reddit, but in case you don't lurk here's some more folks very excited to hear about this finally coming 😁 https://twitter.com/SeeminglyScienc/status/1595149054610399232
|
Thanks for the kind words :) |
PR Summary
Fixes a few things about hashtable completion:
Example of a scenario that currently fails, but will get fixed by this PR:
Also adds completers for the
Arguments
parameter forInvoke-CimMethod
theFilterHashtable
parameter forGet-WinEvent
the PropertyParameter
for the cim cmdlets and the standard cim completers for Set-CimInstance.This also removes duplicates from member completion in scenarios like
ls | select Attributes,<Cursor here>
this could be split off into its own PR but since it depends on changes made in this PR I thought I may as well include it here.Finally it adds some basic completion support for hashtables used for splatting. The pseudo binding doesn't take the hashtable parameters into account so it's not perfect but IMO some completion is better than none.
Fixes #9239
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).