Skip to content

Add external API part 1 #2799

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

Merged
merged 5 commits into from
Jul 13, 2020

Conversation

TylerLeonhardt
Copy link
Member

PR Summary

This adds the foundation of the API that external extensions will use. Once @rjmholt is done with the work in PSES to better handle the runspace, we can add part 2 which will execute PowerShell script in the PowerShell runspace.

Here are the messages that are encompassed in this PR:

RegisterExternalExtension

I want to be able to track what extensions are leveraging this feature.
In order to do this,
we can have them register first.
With that,
they'll get a UUID back that they use in any future request.
It's like an API KEY of sorts...

vscode.commands.executeCommand(
    "PowerShell.RegisterExternalExtension",
    "ms-vscode.PesterTestExplorer" // the name of the extension using us
    "v1"); // API Version. I made it a single number because I don't want to be in the business of maintaining 1.x.x versions
// returns string session uuid

UnregisterExternalExtension

This will remove the session. Not a very useful API, but it's there.

vscode.commands.executeCommand(
    "PowerShell.UnregisterExternalExtension",
    "uuid"); // the uuid from above for tracking purposes
// returns boolean

GetPowerShellVersionDetails

This will fetch the version details of the PowerShell used to start VS Code.

vscode.commands.executeCommand(
    "PowerShell.GetPowerShellVersionDetails",
    "uuid"); // the uuid from above for tracking purposes

This will return an object with common properties that might be useful:

{
    exePath: "/usr/local/bin/pwsh",
    verison: "7.1.0-preview.4",
    displayName: "PowerShell Preview (x64)", // comes from the Session Menu
    architecture: "x64"
}

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.

  • PR has a meaningful title
  • Summarized changes
  • PR has tests
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

@TylerLeonhardt TylerLeonhardt requested a review from rjmholt July 8, 2020 19:30
Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really like the tests

for (const [_, externalExtension] of ExternalApiFeature.registeredExternalExtension) {
if (externalExtension.id === id) {
const message = `The extension '${id}' is already registered.`;
log.writeWarning(message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a log.writeError -- should we use that if we're throwing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that the logs are really for us, not the extension author. To help the extension author a bit, I used warning... but technically it's user error, not our issue. So I felt it didn't deserve writeError. But happy to change it. I don't care too much.

RETURNS:
string session uuid
*/
vscode.commands.registerCommand("PowerShell.RegisterExternalExtension", (id: string, apiVersion: string = 'v1'): string => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this object sets its fields in the constructor, it might be nicer if each command were a method on the class (and then the comments describing them could be on those methods maybe), and then have the commands array just point to those

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed.

@TylerLeonhardt
Copy link
Member Author

TylerLeonhardt commented Jul 8, 2020

btw this message will allow me to delete this file :) in the test explorer code

https://github.com/TylerLeonhardt/vscode-powershell-test-adapter/blob/master/src/process.ts

@TylerLeonhardt
Copy link
Member Author

ok merge time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants