-
Notifications
You must be signed in to change notification settings - Fork 48
#39: implement pip commandlet #1597
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
Conversation
Pull Request Test Coverage Report for Build 19944545054Details
💛 - Coveralls |
|
I've tested the pip command on my WSL2.0 within intellij and on my Windows 11 machine. Works fine so far. WSL logs: Windows logs: |
added execute permissions to binaries added extra echo for pip to uv binaries replaced cmd content with bash
jan-vcapgemini
left a comment
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.
@maybeec thanks for adding this missing commandlet. Looks good to me, ready for review.
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.
@maybeec thank you very much for your PR. 👍
I had a long discussion already about our plans in the dailies with @jan-vcapgemini that we never documented in the story. Sorry for that and hence my review comments requesting some rework.
For my curiosity: I am happy that you upgraded setup-java action lib. Is this required for this PR and especially why did you change the JDK distribution to zulu?
I do not mind this change, I just want to understand if this is related to the change or if it is a general best practice you learned that I might be missing.
BTW: On our long learning with GitHub we discovered that we often end up with PRs that grow too big and are hard to review (I am not talking about this PR, but PRs changing 100+ files, what we have seen several times). Still I sometimes tend to include refactorings in a bugfix and maybe also include unrelated cleanups that I hit on the way. However, on the long run, we learned that ideally we create small PRs that only focus on a single bug or feature. If we want to also update an action or cleanup some code we hit, it is best, to just create another PR for it.
Thanks for all your input: I clearly see that I need to take some time to write way more documentation about all these things. Ideally in reviews, I only need to paste links to existing documentation if something is not as desired (most likely not in your case but more for junior developers contributing PRs).
cli/src/test/resources/ide-projects/pip/_ide/urls/pip/pip/dependencies.json
Outdated
Show resolved
Hide resolved
cli/src/test/resources/ide-projects/pip/_ide/urls/uv/uv/0.5.11/mac_x64.urls
Outdated
Show resolved
Hide resolved
cli/src/test/resources/ide-projects/pip/repository/uv/uv/default/uv
Outdated
Show resolved
Hide resolved
cli/src/test/resources/ide-projects/pip/repository/uv/uv/default/uv.cmd
Outdated
Show resolved
Hide resolved
Removed duplicate entry for issue devonfw#39 in the changelog.
…ommandlet
- Refactored Pip.java to extend LocalToolCommandlet instead of DelegatingToolCommandlet
- Created PipRepository extending ArtifactToolRepository for PyPI version resolution
- Created PipArtifact and PipArtifactMetadata data classes
- Created PypiJsonObject record for parsing PyPI JSON API responses
- Added getPipRepository() to IdeContext interface and AbstractIdeContext
- Created PipRepositoryMock for testing with WireMock
- pip installation now delegates to 'uv pip install pip=={version}'
- pip execution runs through 'uv pip' instead of direct pip binary
- Consolidated test resources and fixed test configuration
Addresses review comments from PR devonfw#1597
Summary of Changes (addressing review feedback)Based on the review comments, I've refactored the pip commandlet from Key Changes:
Test Results:
The implementation now follows the reviewer's requirements to make pip a |
See my comment: #1597 (comment) I could get rid of it again if you like, but it's also so tiny that I feel it's still in the "okish" area as I fully agree to the point you are making to provide refactorings in separate PRs is way more healthy. |
hohwille
left a comment
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.
@maybeec thanks for the rework. You perfectly addressed my review-feedback. Great job implementing the PipRepository which is surely not a simple task to get right. Also the mocking for tests is done perfectly 🥇
cli/src/test/resources/ide-projects/pip/_ide/urls/pip/pip/dependencies.json
Outdated
Show resolved
Hide resolved
|
|
||
| // Create installation path and write version file | ||
| this.context.getFileAccess().mkdirs(installationPath); | ||
| this.context.writeVersionFile(resolvedVersion, installationPath); |
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.
is the installationPath then software/pip that will contain nothing but the .ide.software.version file for pip then? Not wrong but maybe confusing. For npm based stuff we do not write these .ide.software.version and ask npm for the installed version via CLI. We could also do that here via pip list.
However, to keep it constructive and efficient, I would conclude that you have done a great job with this PR and this is ready for merge now. I can add further perfection with separate PRs once I find some time.
There is nothing wrong here.
|
Sorry, that I broke your PR. I am just assuming what we have as "standards" in IDEasy for commandlets but that is currently not the case for |
|
I created PR #1639 that can replace this one. Thanks again for the great work. |
This PR fixes #39
Implemented changes:
Pipcommandlet that delegates toUvcommandlet by prepending "pip" to argumentsuv pipinternallyPipTestto verify basic functionalityTechnical Details:
The pip commandlet extends
DelegatingToolCommandlet<Uv>and overridesrunTool()to prepend "pip" to the arguments before delegating to UV. This means when users runide pip install <package>, it internally executesuv pip install <package>.This implementation follows the hint provided in the issue that pip should be implemented as a delegate to
uv pip.Checklist for this PR
mvn clean testlocally all tests pass and build is successful (test errors are pre-existing)#«issue-id»: «brief summary»In Progressand assigned to you or there is no issue (might happen for very small PRs)internalChecklist for tool commandlets
«TOOL»_VERSIONand«TOOL»_EDITIONare honored by your commandlet (handled by delegation to UV)