#1090: replace picocli #1355
#1090: replace picocli #1355
Conversation
…mPath, establish software repo and symlinking, MacOS workaround
|
|
||
| private CliArgument initContext(CliArgument first) { |
There was a problem hiding this comment.
Please provide javadoc for this method.
There was a problem hiding this comment.
In general we seem not to be on the same page about writing JavaDoc for private things.
The method initContext will initialize the context. Okay, why we need to pass the CliArgument and get CliArgument back is not obvious from the signature. I can write JavaDoc for this but my PoV is that you can read the code if you want to know this. A problem with adding JavaDoc is that it might not get maintained if the code gets changed and internal private methods tend to change often. A wrong JavaDoc is worse than no JavaDoc.
Did you ever read the book about "clean code"? If no, I would strongly recommend it...
Anyhow in this case, I am happy to add the JavaDoc and address your feedback as actually the @return would be helpful.
There was a problem hiding this comment.
I'm asking for javadocs because of IDE autocomplete and documentation functionalities. The more (maintained) javadocs we have for methods like these, the more the IDE's will help us to get through faster. I don't like to read each method body multiple times to understand what my params need to be and what will be returned and how.
I've not read "clean code" yet. Might give it a go later.
|
|
||
| private void add(String name, Property<?> property, boolean alias) { |
There was a problem hiding this comment.
Please provide javadoc for this method.
There was a problem hiding this comment.
This is clearly a point where I would strongly disagree.
There is a protected add method what is the API for commandlets inheriting from this class.
It does have JavaDoc. This private method only exists to avoid code redundancy and the only place where it is called is the protected add method. So IMHO a user would only be interested in the API and if he digs in the implementation of the protected add method and will stumble over the private one he can easily read the 12 lines of code that are purely trivial.
I would rather challenge that the existing JavaDoc like @param property the {@link Property} to register. might be a little bit short and could explain what "registration" means here. Ideally we will live a culture where every developer takes responsibility of the code and feels invited to improve JavaDoc, add JUnit coverage, etc. to all existing code.
I try my best to produce high quality code but only have very few hours a day for management, contributing and coding to this project. So sure I could write more JavaDoc, more JUnits (that is IMHO even a much bigger pain point currently).
Do not get me wrong:
Thank you for your review and feedback. I just wanted to make a clear point to match expectations.
There was a problem hiding this comment.
Thanks for pointing that out. I'm asking for javadocs only so that I don't have to read the method body multiple times as mentioned before. Aren't there tools which could automate the javadoc and param description using AI yet?
| */ | ||
| public boolean isIdeHomeRequired() { | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Maybe move to a constant.
There was a problem hiding this comment.
You seemed to miss that this method gets overridden by commandlets that do not require IDE_HOME.
So by default this is true and invoking the commandlet will fail if IDE_HOME was not found.
However, commandlets like HelpCommandlet should still be able to be invoked.
I thought the JavaDoc would explain this. Feel invited to improve the JavaDoc (e.g. add "Override this method in your commandlet if it does not require IDE_HOME being set").
| // if [ -e "${target_dir}/Applications" ] | ||
| // then | ||
| // rm "${target_dir}/Applications" | ||
| // fi |
There was a problem hiding this comment.
Oh great point. That is also from the "clean code" book. Never keep outcommented code. There is git for the history.
However, this was some kind of TODO for me.
This comes from here and I could not see or remember what this is for.
If we still need this, it is a bug that the Java code is missing this behavior.
After all our Java implementation (IDEasy) is still in prototype state :)
There was a problem hiding this comment.
I would recommend to add a // TODO: comment with a link to an issue instead of keeping outcommented code.
| /** | ||
| * @param rootDir the {@link Path} to the root directory. | ||
| * @param fileAccess the {@link FileAccess} instance. | ||
| * @param systemInfo the {@link SystemInfo} instance. | ||
| * @param logger the {@link IdeLogger} instance. | ||
| * @return the {@link ToolInstallation#linkDir() link directory}. | ||
| */ | ||
| Path findLinkDir(Path rootDir) { |
There was a problem hiding this comment.
Nice catch. I moved these parameters to the constructor. Hence, they have to be removed from the JavaDoc.
Just did this: devonfw/IDEasy@9789dbb
For the record: As I said if you write lots of JavaDoc you may end up with wrong JavaDoc if it is not properly maintained. Eclipse gives me warnings for this but only for public and protected methods. So I missed this one. Thanks for your finding.
There was a problem hiding this comment.
For that I would recommend: https://www.jetbrains.com/help/idea/javadocs.html#fix-javadoc
|
|
||
| private static String computeId(String url) { |
There was a problem hiding this comment.
Please add some javadoc.
There was a problem hiding this comment.
Same here: This method computes a unique ID from the given URL. The signature is already saying all you need. No need for private JavaDoc here.
There was a problem hiding this comment.
In this case I would like to know in which format the String of the URL has to be. Otherwise I would use URL type to prevent issues with bad URL's.
#1090