#1167: automatic project import vscode - #2263
Conversation
Coverage Report for CI Build 31963676067Warning No base build found for commit Coverage: 72.916%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats💛 - Coveralls |
…tic-project-import-vscode
hohwille
left a comment
There was a problem hiding this comment.
@QuangAnhLe thanks for your PR. You got the general idea right but this cannot work as is yet. I left some review comments to address before we can merge. Thanks 👍
| @Override | ||
| public void importRepository(Path repositoryPath) { | ||
| CommandletManager commandletManager = this.context.getCommandletManager(); | ||
| for (Entry<Class<? extends LocalToolCommandlet>, String> entry : BUILD_TOOL_TO_TEMPLATE.entrySet()) { | ||
| LocalToolCommandlet buildTool = commandletManager.getCommandlet(entry.getKey()); | ||
| Path buildDescriptor = buildTool.findBuildDescriptor(repositoryPath); | ||
| if (buildDescriptor != null) { | ||
| String templateFilename = entry.getValue(); | ||
| LOG.debug("Found build descriptor {} so merging template {}", buildDescriptor, templateFilename); | ||
| mergeSettings(repositoryPath, templateFilename); | ||
| return; | ||
| } | ||
| } | ||
| LOG.warn("No supported build descriptor was found for project import in {}", repositoryPath); | ||
| } |
There was a problem hiding this comment.
Correct, but we are coming back to copy&paste culture.
My suggestion would be to move this entire method to IdeToolCommandlet.
Then instead of accessing the map from constant (BUILD_TOOL_TO_TEMPLATE), simply use getBuildTool2TemplateMap() add a method in the same class:
protected Map<Class<? extends LocalToolCommandlet>, String> getBuildTool2TemplateMap() {
return Map.of();
}
Then Intellij and Vscode can override this method and return their constant.
BTW: Why was importRepository API Signature declared in ToolCommandlet - this only makes sense for IdeToolCommandlet so I would even move the declaration there and fix the usage in RepositoryCommandlet accordingly as everything else does not make any sense to me.
| /** | ||
| * Merges the VSCode settings template into the workspace's {@code .vscode/settings.json}. | ||
| * | ||
| * @param repositoryPath the {@link Path} to the repository to import. | ||
| * @param configFilePath the filename of the config file (e.g. {@code settings.json}). | ||
| */ | ||
| private void mergeSettings(Path repositoryPath, String configFilePath) { | ||
| Path templatePath = this.context.getSettingsPath().resolve(TEMPLATE_LOCATION); | ||
| Path templateFile = templatePath.resolve(configFilePath); | ||
| if (!Files.exists(templateFile)) { | ||
| throw new CliException( | ||
| "Cannot import project into workspace: template file not found at " + templateFile + "\n" | ||
| + "Please do an upstream merge of your settings git repository."); | ||
| } | ||
| Path workspacesPath = this.context.getIdeHome().resolve(IdeContext.FOLDER_WORKSPACES); | ||
| Path workspacePath = this.context.getFileAccess().findAncestor(repositoryPath, workspacesPath, 1); | ||
| if (workspacePath == null) { | ||
| throw new CliException( | ||
| "Cannot import project into workspace: could not find workspace from " + repositoryPath); | ||
| } | ||
| JsonMerger jsonMerger = new JsonMerger(this.context); | ||
| EnvironmentVariables environmentVariables = getVscodeEnvironmentVariables(workspacePath.relativize(repositoryPath)); | ||
| Path vscodeFolder = workspacePath.resolve(FOLDER_VSCODE); | ||
| Path workspaceFile = vscodeFolder.resolve(configFilePath); | ||
|
|
||
| // Ensure .vscode folder exists | ||
| this.context.getFileAccess().mkdirs(vscodeFolder); | ||
|
|
||
| // Merge template into workspace settings (template acts as "setup" for creation, also as "update" for variable resolution) | ||
| jsonMerger.merge(templateFile, templateFile, environmentVariables, workspaceFile); | ||
|
|
||
| LOG.debug("Merged VSCode settings into {} for repository at {}", workspaceFile, repositoryPath); | ||
| } |
There was a problem hiding this comment.
The same here. Seems like a complete copy of Intellij.mergeConfig.
Move and centralise in IdeToolCommandlet. Instead of using TEMPLATE_LOCATION constant, build it dynamically.
p.s.: I already gave that feedback for the initial Intellij project import to avoid such redundancies since I saw all this coming - but it seems my review comments where not properly addressed (see PR #1466).
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "java.project.rootPath": "$[PROJECT_PATH]" | |||
There was a problem hiding this comment.
This is not the way how to "import" a project.
Please note that you can import multiple projects into the same workspace and here you only have a single property that the 2nd import would override from the 1st one.
Further, if that template is complete and correct, you first need to create a PR for ide-settings that has to be merged before this PR can be merged as otherwise we will always end up in the CliException that you are throwing if the template file is not present.
| * https://github.com/devonfw/IDEasy/issues/2056[#2056]: Replaced progress bar with general progress information for installing plugins in VSCode | ||
| * https://github.com/devonfw/IDEasy/issues/1659[#1659]: Abort runTool with warning when global tool installer runs in background | ||
| * https://github.com/devonfw/IDEasy/issues/1720[#1720]: Integrate SoapUI | ||
| * https://github.com/devonfw/IDEasy/issues/1167[#1167]: Automatic project import for vs code |
There was a problem hiding this comment.
Please move this entry up to the latest release.
This PR fixes #1167
Implemented changes:
Adds automatic VSCode workspace settings import for Maven and Gradle projects. When a repository is imported, the Vscode.importRepository() method:
Testing instructions
mvn -Dtest="VscodeTest#testVscodeMvnRepositoryImport,VscodeTest#testVscodeGradleRepositoryImport" test -pl cli
mvn -Dtest="VscodeTest" test -pl cli
${PROJECT_PATH} replaced with the
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internal