Skip to content
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

#751: Moved postInstall() and changed tool installation message #763

Merged

Conversation

leonrohne27
Copy link
Contributor

Fixes: #751

Implements:

  • Moved the method postInstall() from GlobalToolCommandlet to LocalToolCommandlet
  • Changed the installation message so that the user is not getting confused

Moved the method postInstall from GlobalToolCommandlet to LocalToolCommandlet and changed the installation message so that the user is not getting confused
@leonrohne27 leonrohne27 self-assigned this Nov 14, 2024
Copy link
Contributor

@jan-vcapgemini jan-vcapgemini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Ready for review.

@jan-vcapgemini jan-vcapgemini changed the title #751: Moved postInstall() and changed toll installation message #751: Moved postInstall() and changed tool installation message Nov 14, 2024
@hohwille hohwille added this to the release:2024.12.001 milestone Nov 15, 2024
@hohwille hohwille self-assigned this Nov 15, 2024
Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leonrohne27 thanks for your PR. You solved the second task of #751 nicely. 👍
For the first task there is a slight misunderstanding - sorry, if I was not precise enough:

Move postInstall() to LocalToolCommandlet and remove it from [Global]ToolCommandlet:

I expect that the postInstall method(s) (with its body/implementation) gets moved to LocalToolCommandlet. If GlobalToolCommandlet does not support postInstall, then it does not make sense anymore if it has a method postInstall.
It is fully correct that you removed the invocation of postInstall() from GlobalToolCommandlet but you also need to move postInstall() and postInstall(boolean) from ToolCommandlet to LocalToolCommandlet.
Then sub-classes of GlobalToolCommandlet cannot override postInstall anymore and since we always use @Override annotations in such case, we would even get compile errors if we missed something.
Please have a look and update your PR. Thanks.

@coveralls
Copy link
Collaborator

coveralls commented Nov 18, 2024

Pull Request Test Coverage Report for Build 11888068434

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 35 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.005%) to 67.287%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/tool/GlobalToolCommandlet.java 3 2.04%
com/devonfw/tools/ide/tool/LocalToolCommandlet.java 13 86.57%
com/devonfw/tools/ide/tool/ToolCommandlet.java 19 77.49%
Totals Coverage Status
Change from base Build 11853076778: 0.005%
Covered Lines: 6400
Relevant Lines: 9170

💛 - Coveralls

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leonrohne27 thanks for rework, ready for merge now 👍

@hohwille hohwille merged commit d2a3760 into devonfw:main Nov 19, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Small cleanup of GlobalToolCommandlet from PR #684
4 participants