-
Notifications
You must be signed in to change notification settings - Fork 498
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
Show-Command explorer v1 #1406
Show-Command explorer v1 #1406
Conversation
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.
🎉🎉🎉
I just noticed all the comments from Tyler. Some of them were resolved on their own 😀. Once the refresh works... |
v1 of this is ready for review. One thing that is bothering me is: when you select a command, right click and choose to Insert Command, it inserts at the current location in editor, but does not focus editor, and it selects the entire command that is insert instead of putting cursor at the end of the inserted command. |
src/features/GetCommands.ts
Outdated
} | ||
|
||
} | ||
function toCommand(command: any): Command { |
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.
Ideally should be able to define (and export) an ICommand
interface to describe the properties you require
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.
I've defined an ICommand interface. I'm not sure on the need to export it. Is there a use case where we need to access it outside of GetCommands?
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.
This is looking GREAT @corbob! I also had a couple comments and questions :) I'm really excited to get this in!
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 just have a couple questions and you should address the rest of Rob's comments too :)
I didn't see the "powerShell/getCommand" message used at all in this PR even though it's in the PSES PR. Is that on purpose? |
The "powerShell/getCommand" message was added in PSES as a precursor to that we will need it on this side, but the next step I believe will be mostly on this side. However, based on the suggestion of a single message I think it's the better way. |
Changing PR back to WIP to address the refactoring @tylerl0706 suggested. |
@corbob, since "powerShell/getCommand" is not used in this feature (yet). You could just remove the message from the PSES PR so we can get this in 😊 Then V2 of the feature could do the refactoring. If you want to do the refactoring now, that's great but I won't block on that 😊 |
I think I've got most of the stuff. Only thing I'm not 100% on is some of the comment based documentation parts. |
First pass at bringing a TreeView into vscode-powershell. Built currently failing on unrelated code...
First pass at bringing a TreeView into vscode-powershell. Built currently failing on unrelated code...
Updated from previous name incarnation..
Require an svg for the activity bar. Taken from https://github.com/Microsoft/vscode-extension-samples/tree/master/tree-view-sample/media (MIT Licensed I think...)
PSES isn't returning a string. Make sure we're expecting it. Allowing for console.log for debugging. These will all be removed prior to shipping.
Unable to currently update the list. If you run PowerShell.GetCommands prior to opening the treeview, it is populated with your commands. Need to get it to update.
Other icons appear to be 32 px square. This one was 50. Adjusting size to 32.
Cleaning up the tslinter hints. Remove CommandNode, was part of a failed experiment to get the tree refresh working.
Correct casing of some things. Adjust names to increase chance of uniqueness.
Thanks to glennsarti for finding that the API expects to call back to a particular variable.
Add Insert Command Add Right Click Options to View Reset PSES Logging to Normal Update OnlineHelp to take an item from the treeview. Pass that to online help PSES handler.
Adding functionality to PSES to get all commands (Name and Module Name), or get all of a single command. This solves performance issues with getting all of all of them.
Refactor into a single command. If nothing is passed, it will return all objects.
Clean up TODO comments Clean up request type naming. White space cleanup
OnlineHelp has been deprecated as of 1.9.0.
Clean up some naming and whitespace.
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.
Just took a sec to look at this again. It looks good to me!
Hopefully we can give it and the PSES PR a quick test soon and merge it in.
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!
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!
PR Summary
Implement a Show-Command type panel for #1405
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
x
between the square brackets.Please mark anything not applicable to this PR
NA
.WIP:
to the beginning of the title and remove the prefix when the PR is ready