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

#17: Implement tool commandlet for dotnet #263

Merged
merged 45 commits into from
Apr 29, 2024

Conversation

ndemirca
Copy link
Contributor

@ndemirca ndemirca commented Mar 22, 2024

Closes #17

@coveralls
Copy link
Collaborator

coveralls commented Mar 27, 2024

Pull Request Test Coverage Report for Build 8849659184

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 39 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.01%) to 59.823%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/context/IdeContext.java 1 45.45%
com/devonfw/tools/ide/commandlet/CommandletManagerImpl.java 3 89.53%
com/devonfw/tools/ide/step/Step.java 14 36.36%
com/devonfw/tools/ide/process/ProcessContextImpl.java 21 76.08%
Totals Coverage Status
Change from base Build 8828327733: 0.01%
Covered Lines: 4604
Relevant Lines: 7408

💛 - 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.

@ndemirca thanks for your PR. Also nice that you added this DEFAULT_SILENT mode properly and added all the features from the old bash code.
What I missed out to explain either explicitly in the story or clearly enough in the team.
With IDEasy we are decoupleling from devonfw so commands like devon java create, devon java migrate and devon dotnet create will not be supported by IDEasy.
If we would like to have ide dotnet c[reate] supported we also should have create as an optional keyword property with alias c so it can be supported by auto-suggestion, help, etc.
However, my conclusion would be to remove all these devon4Net methods and postInstall but keep the DEFAULT_SILENT mode that is a nice feature what we will need sooner or later anyhow.
I am deeply sorry for the extra effort you had for this due to my mistake.
I will explain this to the entire team in the next KT session and please remind me in case I might forget it ;)

@ndemirca ndemirca self-assigned this Apr 25, 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.

@ndemirca thanks for the cleanup. PR is ready for merge now 👍
I left comments for files that are IMHO obsolete. If yes, you can delete them.
Also with removing the devon4net stuff and the according tests, you also removed the test of running dotnet commandlet (beyond install). That was not my intention in the last review...

@ndemirca ndemirca marked this pull request as draft April 29, 2024 12:18
@ndemirca ndemirca marked this pull request as ready for review April 29, 2024 16:10
@hohwille
Copy link
Member

Sorry, it seems I marked to many files for removal as it seems.

[ERROR] Failures:
[ERROR] DotNetTest.dotnetShouldRunExecutableForWindowsSuccessful:61->checkExpectedOutput:81->AbstractIdeContextTest.assertLogMessage:131->AbstractIdeContextTest.assertLogMessage:154 [INFO-Log messages]
Expecting LinkedList:
["You are not inside an IDE installation: /home/runner/work/IDEasy/IDEasy/cli/target/test-projects/dotnet/project"]

So the project is not detected as IDE_HOME anymore:

private boolean isIdeHome(Path dir) {
if (!Files.isDirectory(dir.resolve("workspaces"))) {
return false;
} else if (!Files.isDirectory(dir.resolve("settings"))) {
return false;
}
return true;
}

I assume at least a workspaces/main/.gitkeep file needs to be present so the workspaces folder exists.

@hohwille
Copy link
Member

I assume at least a workspaces/main/.gitkeep file needs to be present so the workspaces folder exists.

Mhm. I am not getting it:
Both settings and workspaces is still present in your test project. Why does it not detect IDE_HOME then?

@hohwille
Copy link
Member

Mhm. I am not getting it: Both settings and workspaces is still present in your test project. Why does it not detect IDE_HOME then?

OK, now I get it. You fixed it already while I was writing this and with your latest fix the build should now get green. Great.

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.

@ndemirca thanks for making this perfect. Ready for merge now 👍

@hohwille hohwille merged commit bab8953 into devonfw:main Apr 29, 2024
3 checks passed
@hohwille hohwille added this to the release:2024.05.001 milestone Apr 29, 2024
@hohwille hohwille added the story-review marks PRs that will be presented in the sprint-review label May 3, 2024
@tobka777 tobka777 added reviewed Marks PRs that have been presented in the sprint-review meeting or that do not need to be presented. and removed story-review marks PRs that will be presented in the sprint-review labels May 7, 2024
@ndemirca ndemirca deleted the feature/17-NewDotNetToolCommandlet branch May 29, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewed Marks PRs that have been presented in the sprint-review meeting or that do not need to be presented.
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Implement ToolCommandlet for .NET
4 participants