-
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
#183: removed isInstalledVersion() #184
#183: removed isInstalledVersion() #184
Conversation
this method was only used once and caused the duplicate msg if the tool was already installed.
Pull Request Test Coverage Report for Build 7612283729
💛 - Coveralls |
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.
Indeed we have this log message duplicated:
this.context.debug("Version {} of tool {} is already installed at {}", resolvedVersion, this.context.debug("{} is already installed at {}", this.tool, binaryPath); this.context.level(level).log("Version {} of tool {} is already installed", installedVersion,
So thanks @MattesMrzik for noticing and taking action to improve. 👍
However, using the silent
flag to determine the log-level is an intended behavior we have taken from devonfw-ide
.
The idea is as following:
$ devon mvn setup
Version 3.9.6 of tool mvn is already installed
This is the intended default behavior. But also consider this:
$ devon --debug mvn setup
Version 17.0.8 of tool java is already installed
Version 3.9.6 of tool mvn is already installed
By default the user would not expect to get this info about java and that would come every time we run mvn
if we log it statically on info
or it would come never by default if we log statically on debug
so devon java setup
would give no feedback at all by default what would be confusing:
$ devon java setup
$
Your change is fine, but you should also add the dynamic logging part from isInstalledVersion
method to ToolCommandlet
as a protected method that is used by Local/GlobalToolCommandlet. I missed this on the review of #113
UPDATE: I just see that in GlobalToolCommandlet we do not have a version
so you could still do what I suggested but allow GlobalToolCommandlet
to pass null
as version parameter and log accordingly.
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 like @hohwille said we need this dynamic logging otherwise we don't get any output when not in debugger mode. If I'm not mistaken you can move isInstalledVersion to ToolCommandlet and use it both in Global and Local, maybe call it isInstalled since Global Tools don't have any version.
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 to me 👍
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.
OK, PR does not avoid the redundancies but functionally now correct and ready for merge.
I commented on #52 because of the pointless line-wrapping changes, but this can now be merged. Thanks again 👍
…-already-installed
Fixes #183.
isInstalledVersion()
was only used once and caused the duplicate msg if the tool was already installed.