-
Notifications
You must be signed in to change notification settings - Fork 511
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
Add external API part 1 #2799
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.
Really like the tests
src/features/ExternalApi.ts
Outdated
for (const [_, externalExtension] of ExternalApiFeature.registeredExternalExtension) { | ||
if (externalExtension.id === id) { | ||
const message = `The extension '${id}' is already registered.`; | ||
log.writeWarning(message); |
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 there a log.writeError
-- should we use that if we're throw
ing?
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.
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.
src/features/ExternalApi.ts
Outdated
RETURNS: | ||
string session uuid | ||
*/ | ||
vscode.commands.registerCommand("PowerShell.RegisterExternalExtension", (id: string, apiVersion: string = 'v1'): string => { |
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.
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
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 like that
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.
changed.
btw this message will allow me to delete this file :) in the test explorer code |
Co-authored-by: Robert Holt <[email protected]>
d46e886
to
a468b47
Compare
ok merge time |
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...
UnregisterExternalExtension
This will remove the session. Not a very useful API, but it's there.
GetPowerShellVersionDetails
This will fetch the version details of the PowerShell used to start VS Code.
This will return an object with common properties that might be useful:
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