-
Notifications
You must be signed in to change notification settings - Fork 24
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
#37: Tool commandlet for visual studio code #538
#37: Tool commandlet for visual studio code #538
Conversation
Pull Request Test Coverage Report for Build 11019972842Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
cli/src/test/java/com/devonfw/tools/ide/tool/vscode/VscodeTest.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/vscode/Vscode.java
Outdated
Show resolved
Hide resolved
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.
@aBega2000 thanks for your PR. 👍
I have nothing to add to what @slskiba already left as review comments. Please address these review comments. Then we can resolve and merge.
cli/src/test/java/com/devonfw/tools/ide/tool/vscode/VscodeTest.java
Outdated
Show resolved
Hide resolved
…ttps://github.com/aBega2000/IDEasy into feature/37-tool-commandlet-for-visual-studio-code
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.
@aBega2000 Thanks for your rework and improvements. 👍
Still it turns out some details are not yet clear and your implementation needs further rework.
I left several review comments for you to address.
As I cannot put a review comment on an empty file:
Please try to avoid inconsistent test-data except for cases where you explicitly want to test how the software behaves in case or corrupt data.
A Empty file is not a valid JSON file.
Simply put {}
into extensions.json
and you are safe.
Your current "hack" code migrated from bash would also in this case insert `"extensions": "plugin1,plugin". This may let your JUnit pass but this is still invalid JSON and the real VSCode application would complain about it.
cli/src/main/java/com/devonfw/tools/ide/tool/vscode/Vscode.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/vscode/Vscode.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/vscode/Vscode.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/vscode/Vscode.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/vscode/Vscode.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/vscode/Vscode.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/plugin/PluginBasedCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/vscode/Vscode.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/tool/vscode/Vscode.java
Outdated
Show resolved
Hide resolved
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.
@aBega2000 thanks for your rework. Looks very good already. 👍
One review comment was still not completed and I added some small hints for final improvement. We can merge when these are resolved.
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.
Looks good so far. I've added some CRs to resolve.
cli/src/main/java/com/devonfw/tools/ide/tool/plugin/PluginBasedCommandlet.java
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/tool/vscode/VscodeTest.java
Outdated
Show resolved
Hide resolved
…ttps://github.com/aBega2000/IDEasy into feature/37-tool-commandlet-for-visual-studio-code
cli/src/main/java/com/devonfw/tools/ide/tool/vscode/Vscode.java
Outdated
Show resolved
Hide resolved
CHANGELOG entry needs to be added for this PR still |
added vscode to changelog adjusted vscode install test (added a recommendation to extensions.js and check if new recommendation was added)
…-code # Conflicts: # CHANGELOG.adoc
…-code # Conflicts: # CHANGELOG.adoc
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.
@jan-vcapgemini thanks for taking over this PR and resolving the last review comments. 👍
Still added some suggestions but will simply apply and then merge.
Fixes #37