#1953: Implement base functionality of Cleanup Commandlet - #1957
#1953: Implement base functionality of Cleanup Commandlet#1957areinicke wants to merge 27 commits into
Conversation
Coverage Report for CI Build 28829571525Coverage decreased (-0.05%) to 72.013%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions7 previously-covered lines in 1 file lost coverage.
Coverage Stats💛 - Coveralls |
Caylipp
left a comment
There was a problem hiding this comment.
I tested the cleanup commandlet locally.
For my manual test, I installed additional versions of gh and java and then ran ide cleanup. Only the extra unused versions were deleted, while the versions still referenced by a project were kept. So the basic deletion functionality worked for me.
I noticed an issue in ide shell where duplicate entries appeared on a second run. However, this issue has already been fixed.
Overall, the implementation looks good to me and my manual test worked. Very good!
hohwille
left a comment
There was a problem hiding this comment.
@areinicke thanks for your PR. Great that you divided the very large story and created a very reasonable starting point as first implementation that totally makes sense to me.
Also the logging as preview what will be deleted and the user confirmation looks good to me. Great job 👍
I left quite some review comments due to some conventions and design issues.
I hope you consider it as appreciation to learn some things to further advance your Java coding style and not as nasty picky pointless extra rework ;)
| private static class IdeTool { | ||
| String toolName; | ||
| private final Path path; | ||
| private final List<String> usedBy = new java.util.ArrayList<>(); |
There was a problem hiding this comment.
I am slightly confused why we have this usedBy in all 3 inner classes.
If we really want to do that, we could even use OOP and derive the 3 classes from an abstract class with a getUsedBy() and a nice centralised JavaDoc that is then inherited and reused by all 3 classes.
But IMHO we only need to track what tool+edition+version combination is used.
If all unused ones are deleted and we end up with en empty edition folder this does not really waste disc-space and we can still check that after deletion we also delete the parent folder if it became empty (and if we delete the edition folder we can also delete the version folder if that is now also empty).
There was a problem hiding this comment.
BTW: Generic types like String or Number always need explanation. So if you see List<String> you do not know what this is unless you have further JavaDoc or other hints.
If you instead see List<IdeProject> and IdeProject is a class or record with JavaDoc then things immediately get clear. Such IdeProject type could wrap the project name as String but also maybe contain the Path to the project and later could even have methods like getWorkspaces() or whatever - see what @laim2003 is currently implementing for the GUI.
|
|
||
| // assert | ||
| assertThat(context.getIdeRoot().resolve("_ide/software/default/az")).doesNotExist(); | ||
| assertThat(context).logAtSuccess().hasMessage("Unused tools have been deleted successfully."); |
There was a problem hiding this comment.
nice to have: you are just asserting that we logged this message and have deleted something/anything.
We should rather assert that something that is still used was not deleted and something that was unused has been deleted. Otherwise your implementation can be entirely broken but the test may still be green.
hohwille
left a comment
There was a problem hiding this comment.
@areinicke thanks for the update. Nice. 👍
Sorry, that I have one last concern following general Java conventions:
https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html
|
The remaining review feedback has been addressed and all checks are passing. The PR already has an approving review and is ready to merge from my side. |
This PR fixes #1953
Implemented changes:
cleanupcommandlet. Users can runide cleanupto scan for unused tool installations and automatically remove them. A tool is considered as unused if no project links to the tool installation from its software folder at$IDE_ROOT/<project>/softwareor$IDE_ROOT/<project>/software/extra.--fd.ide helpdocumentation for the new commandletTesting instructions
Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:
set-editionandset-versioncommandlet repeatedly.AND
ide helpandide help cleanupto verify that the help documentation works as expected.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