#1754: Add support for VS Code plugin versions#1832
#1754: Add support for VS Code plugin versions#1832areinicke wants to merge 9 commits intodevonfw:mainfrom
Conversation
Coverage Report for CI Build 25362807967Coverage increased (+0.06%) to 70.716%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions20 previously-covered lines in 5 files lost coverage.
Coverage Stats💛 - Coveralls |
|
|
||
| String markerFilePrefix = "plugin" + "." + getName() + "." + getInstalledEdition() + "." + plugin.name(); | ||
| List<Path> markerFiles = fileAccess.listChildren(currentMarkerFilePath.getParent(), | ||
| p -> Files.isRegularFile(p) && p.getFileName().toString().startsWith(markerFilePrefix)); |
There was a problem hiding this comment.
Great addition! However, there is a subtle edge case here with how .startsWith() matches the prefix.
If a user has two plugins installed where one plugin's name is a prefix of another (for example, java and java-extension-pack), installing or updating the shorter one (java) will accidentally delete the marker file for the longer one (java-extension-pack). This happens because "plugin.vscode.null.java-extension-pack" starts with "plugin.vscode.null.java".
To fix this and ensure we don't accidentally delete other plugins' marker files, we should check that the file name either matches the unversioned prefix exactly, or is followed specifically by the .version- delimiter.
| p -> Files.isRegularFile(p) && p.getFileName().toString().startsWith(markerFilePrefix)); | |
| p -> { | |
| String fileName = p.getFileName().toString(); | |
| return Files.isRegularFile(p) && (fileName.equals(markerFilePrefix) | |
| || fileName.startsWith(markerFilePrefix + ".version-")); | |
| }); |
| Properties properties = context.getFileAccess().readProperties(propertiesFile); | ||
| String id = getString(properties, "id", "plugin_id"); | ||
| String url = getString(properties, "url", "plugin_url"); | ||
| String version = getString(properties, "version", "plugin_version"); |
There was a problem hiding this comment.
Users sometimes accidentally leave trailing spaces in .properties files (version=1.44.2 ), which would end up in the CLI command and the marker file.
| String version = getString(properties, "version", "plugin_version"); | |
| String version = getString(properties, "version", "plugin_version"); | |
| if (version != null) { | |
| version = version.trim(); | |
| } |
There was a problem hiding this comment.
ok, I see the issue. But this issue also affects the other fields (e.g., "id"). It would be odd to implement special handling just for this field. We could instead just trim all fields automatically in the getString() method, no? What do you think?
There was a problem hiding this comment.
okay I get what you mean, it is actually better that way to be honest, with that implementation we can keep the error away from all of the fields and not only the version field
| extensionsCommands.add(plugin.id()); | ||
| String extensionInstallTarget = plugin.id(); | ||
| // If a version number was specified, add it to the extension identifier with the format "extensionId@version" | ||
| Boolean versionSpecified = (plugin.version() != null) && !plugin.version().isBlank(); |
There was a problem hiding this comment.
It is better to use the primitive boolean here instead of the boxed Boolean object. This avoids unnecessary object overhead and eliminates the risk of a NullPointerException during unboxing, as this flag will never need to be null.
| Boolean versionSpecified = (plugin.version() != null) && !plugin.version().isBlank(); | |
| boolean versionSpecified = (plugin.version() != null) && !plugin.version().isBlank(); |
ducminh02
left a comment
There was a problem hiding this comment.
Amazing job on this PR Alex! 🚀
The implementation is very clean and It handles the versioned plugin requirement perfectly!
I've left a few comments on some very minor adjustments and one subtle edge case regarding the marker file cleanup logic that we should address to ensure different plugins with similar names don't conflict.
This PR fixes #1754
Implemented changes:
$IDE_HOME/settings/vscode/plugins/)If the specified version is unavailable, the plugin installation will fail.
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