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

#180: commandlet to set tool edition #181

Conversation

MattesMrzik
Copy link
Contributor

@MattesMrzik MattesMrzik commented Jan 12, 2024

Closes #180.

Implemented set-edition, get-edition and list-edition commandlet.

The get-edition assumes that the software of a tool is linked in <IDE_HOME>/software to its source in _ide/software/default/<tool>/<edition>/<current version>.

Added a check when reinstalling tools that not only determines if the newly installed version is different from the current version, but also if the edition is different.

@coveralls
Copy link
Collaborator

coveralls commented Jan 12, 2024

Pull Request Test Coverage Report for Build 7668066283

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

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/commandlet/CommandletManagerImpl.java 3 87.32%
com/devonfw/tools/ide/url/model/UrlMetadata.java 7 80.3%
com/devonfw/tools/ide/tool/ToolCommandlet.java 16 52.59%
Totals Coverage Status
Change from base Build 7667398298: 0.4%
Covered Lines: 3615
Relevant Lines: 6194

💛 - Coveralls

@MattesMrzik
Copy link
Contributor Author

Maybe the get-edition is only relevant for LocalToolCommandlets?

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.

Thanks for your additions and the correction of set and list-version.
It would be nice if you could add some small unit tests for your commands too.

@hohwille hohwille changed the title Feature/#180 feature to set tool edition with commandlet #180: commandlet to set tool edition Jan 26, 2024
…ture/devonfw#180-feature-to-set-tool-edition-with-commandlet

# Conflicts:
#	cli/src/main/java/com/devonfw/tools/ide/commandlet/CommandletManagerImpl.java
#	cli/src/main/java/com/devonfw/tools/ide/process/ProcessResult.java
#	cli/src/main/java/com/devonfw/tools/ide/tool/LocalToolCommandlet.java
#	cli/src/main/resources/nls/Ide.properties
#	cli/src/main/resources/nls/Ide_de.properties
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.

@MattesMrzik thanks for this great PR. This is really good work 👍
I left very few review comments that someone else from the team should take over.
Either we merge this PR as is and create a new issue for the improvements or somebody will adopt the PR - both fine for me. I therefore put it back to team-review that the team can decide how to proceed. If you prefer to create a new issue for the review suggestions, you can then link that issue in a comment and then pass this PR back to me in final review and I can resolve and merge.

ToolCommandlet commandlet = this.tool.getValue();
VersionIdentifier installedVersion = commandlet.getInstalledVersion();
if (installedVersion == null) {
throw new CliException("Tool " + commandlet.getName() + " is not installed!", TOOL_NOT_INSTALLED);
Copy link
Member

Choose a reason for hiding this comment

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

IMHO it would make much more sense to print the configured edition instead of failing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done, adjusted the unit test accordingly. See #194

Comment on lines +353 to +358
String edition = toolPath.toRealPath().getParent().getFileName().toString();
if (!this.context.getUrls().getSortedEditions(getName()).contains(edition)) {
this.context.warning("The determined edition \"{}\" of tool {} is not among the editions provided by IDEasy",
edition, getName());
}
return edition;
Copy link
Member

Choose a reason for hiding this comment

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

If the tool was installed via devonfw-ide this warning will be printed and the printed edition will be software.
IMHO this behavior does not really make sense.
In such case as fallback we could simply print the configured edition what is still better than printing software.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done. See #194

@hohwille hohwille removed their assignment Jan 30, 2024
@hohwille
Copy link
Member

Thanks again @MattesMrzik for this great work. This PR is replaced by #194 thanks to @salimbouch who took over your work to get it to the finishing line.

@hohwille hohwille closed this Feb 12, 2024
@hohwille hohwille added this to the rejected milestone Feb 12, 2024
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.

Feature to set tool edition with commandlet
5 participants