Check extension existence on-the-fly when needed #911
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors extension dependency checking from a static initialization-time check to an on-demand approach. Instead of pre-checking if extensions are installed and setting context variables, the code now checks and prompts for extension installation when the related functionality is actually invoked.
- Replaced static extension existence checks with dynamic on-demand checking
- Added a utility function to handle extension checking, installation prompts, and fallback to manual enablement
- Simplified command visibility conditions by removing the
isModernizationExtensionInstalledcontext check
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/upgrade/utility.ts | Added checkOrInstallExtension function to handle dynamic extension checking and installation prompts |
| src/upgrade/upgradeManager.ts | Refactored to use on-demand extension checking instead of static checks and context setting |
| package.json | Removed isModernizationExtensionInstalled condition from command visibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
why not just revert previous PR directly? |
src/upgrade/upgradeManager.ts
Outdated
|
|
||
| // Commands to be used | ||
| context.subscriptions.push(instrumentOperationAsVsCodeCommand(Commands.JAVA_UPGRADE_WITH_COPILOT, async (promptText?: string) => { | ||
| await checkOrInstallExtension(ExtensionName.APP_MODERNIZATION_UPGRADE_FOR_JAVA, ExtensionName.APP_MODERNIZATION_FOR_JAVA); |
There was a problem hiding this comment.
I want to confirm this part of design. So we still keep upgrade manager command, but you will check the installation of app mod extension?
There was a problem hiding this comment.
Yes; the command depends on another command (javaupgrade.gotoAgentMode) which is provided by AppMod extension.
@wenytang-ms because the feature can still be here without depending on other extensions - this PR is just modifying the parts that have hard dependency on AppMod extension. |
|
also update the package version to release, and changelog |
@wenytang-ms done in 39bc5bd, thanks! |
|
The logic looks good to me. But we have lots of user facing strings for this feature, is it necessary to put them into package.nls.json? Which I think should be required for localization. Maybe @testforstephen can confirm. |
3 participants
No description provided.