-
Notifications
You must be signed in to change notification settings - Fork 34
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
#102: implement update & create commandlet #150
#102: implement update & create commandlet #150
Conversation
…-update-commandlet # Conflicts: # cli/src/main/java/com/devonfw/tools/ide/commandlet/CommandletManagerImpl.java
…-update-commandlet
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.
Just some suggestions about naming of two variables and the setting of a variable
cli/src/main/java/com/devonfw/tools/ide/commandlet/UpdateCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/commandlet/UpdateCommandlet.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.
@salimbouch thanks for your rework and great that you merged implementation of #101 and #102 into this single PR what makes it a lot easier. 👍
I left various review comments for improvement or fixes. Also we need to have a test for CreateCommandlet
as I can only see that UpdateCommandlet
is tested so far.
As this PR has quite some complexity and we have done some review rounds already, could you create a teams meeting for Thursday or Friday so we can align and complete the last steps together to get this to the finishing line and avoid further review ping-pongs?
You have already done a very great job and this is close to the end but it will make things much easier, if we discuss face-to-face.
cli/src/main/java/com/devonfw/tools/ide/commandlet/UpdateCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/context/IdeContext.java
Outdated
Show resolved
Hide resolved
cli/src/test/resources/ide-projects/update/_ide/urls/java/java/17.0.6/linux_x64.urls
Outdated
Show resolved
Hide resolved
cli/src/test/resources/ide-projects/update/project/settings/ide.properties
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/commandlet/CreateCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/commandlet/UpdateCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/main/java/com/devonfw/tools/ide/commandlet/UpdateCommandlet.java
Outdated
Show resolved
Hide resolved
cli/src/test/java/com/devonfw/tools/ide/commandlet/UpdateCommandletTest.java
Outdated
Show resolved
Hide resolved
cli/src/test/resources/ide-projects/update/project/home/.ide/ide.properties
Outdated
Show resolved
Hide resolved
…ndlet.java Co-authored-by: Jörg Hohwiller <[email protected]>
…ndlet.java Co-authored-by: Jörg Hohwiller <[email protected]>
…ndlet.java Co-authored-by: Jörg Hohwiller <[email protected]>
this block must be executed before createVariables()
…salimbouch/IDEasy into 102-implement-update-commandlet
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.
@salimbouch thanks for your excellent rework. Now everything looks good and is ready for merge. 👍
closes #102
closes #101