Skip to content
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

Allows extension install and uninstall command execution to throw exceptions back to caller #88714

Conversation

GabeDeBacker
Copy link
Contributor

This PR fixes #88713

We have a private extension that cannot be placed in the private gallery and we have our own update mechanism for our extension.

Previously it was re-launching VSCode (new process) to install\update the extension but it was pointed out to us that executing the ''workbench.extensions.installExtension' command also works. (The ms-vscode.cpptools extension does this already). However, while ensuring that this technique would work for our extension, I discovered that exceptions from the command implementations in src\vs\workbench\contrib\extensions\browser\extensions.contribution.ts where being "logged" but not "re-thrown" to the command executor.

This change adds an optional parameter to the install and uninstall extension command that will re-throw the exception to the caller if specified.

The optional parameter was added so as to not change the existing behavior (which can be discussed).

@GabeDeBacker
Copy link
Contributor Author

@sandy081 - When creating this fix I did consider just re-throwing the exception rather than adding an additional parameter to the command. I decided to go with an additional parameter rather than change the behavior of the existing command as we probably don't know all the consumers of the API.

@GabeDeBacker GabeDeBacker changed the title Fix formatting Allows extension install and uninstall command execution to throw exceptions back to caller Jan 16, 2020
@sandy081
Copy link
Member

Good point, but I would throw the error without having an additional parameter and better the callers to handle the error if they are not yet which I think is not too many.

@sandy081 sandy081 self-requested a review January 21, 2020 11:31
@sandy081 sandy081 added this to the January 2020 milestone Jan 21, 2020
@GabeDeBacker
Copy link
Contributor Author

@sandy081 - Removed the parameter and re-tested our extensions code. Thanks for the quick reply!

Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

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

Can you please revert the rename changes. Lesser the changes easier to review 😄

@GabeDeBacker
Copy link
Contributor Author

@sandy081 - Certainly, removed renames

@GabeDeBacker
Copy link
Contributor Author

@sandy081 - Any ideas on why the checks are failing? They seem to be failing, well, randomly and not necessarily anywhere near my changes. I'll merge with master (again) and keep trying? Not sure what your requirements are for merging this back to master with these failures occuring.

@GabeDeBacker
Copy link
Contributor Author

@sandy081 - Wondering when this will get merged back in? (Thanks!)

@sandy081 sandy081 merged commit 856a5ad into microsoft:master Jan 24, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants