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

Implement uv pip show #2115

Merged
merged 28 commits into from
Mar 6, 2024
Merged

Implement uv pip show #2115

merged 28 commits into from
Mar 6, 2024

Conversation

ChannyClaus
Copy link
Contributor

Summary

Implementation for #1594
The output will contain only the name, version and location of the packages for now but it should be extendable to include other information in the future.

Quite inexperienced with Rust, so please forgive me if there are things that obviously don't make sense 😭

Test Plan

Added a bunch of unit tests. The exit code behavior matches pip's behavior:

  • When the package is found -> exit code 0
  • When the package isn't found -> exit code 1
  • When one package is found but another isn't -> exit code 0

@ChannyClaus ChannyClaus marked this pull request as draft March 1, 2024 19:32
@ChannyClaus
Copy link
Contributor Author

Dug around a bit and it seems like it's not too difficult to add the additional metadata, particularly Required-by and Requires. Would it be preferred that I include those changes in this PR or in a separate follow-up?

@charliermarsh charliermarsh self-requested a review March 6, 2024 01:39
@charliermarsh charliermarsh added enhancement New feature or improvement to existing functionality compatibility Compatibility with a specification or another tool labels Mar 6, 2024
@charliermarsh charliermarsh added the cli Related to the command line interface label Mar 6, 2024
@charliermarsh
Copy link
Member

@ChannyClaus - Separate PR would be preferred, thank you!

@charliermarsh
Copy link
Member

Marking myself as reviewer on this, sorry for the delay.

@ChannyClaus
Copy link
Contributor Author

ChannyClaus commented Mar 6, 2024

No worries! Thanks for taking a look.

@ChannyClaus
Copy link
Contributor Author

        FAIL [   4.616s] uv::pip_install install_git_public_https_missing_commit

Hmm, seems unrelated, let me try re-triggering via an empty commit.

@ChannyClaus
Copy link
Contributor Author

CI is green again 😄 - seems like it was transient indeed.


/// Show information about one or more installed packages.
pub(crate) fn pip_show(
mut packages: Vec<PackageName>,
Copy link
Member

Choose a reason for hiding this comment

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

@ChannyClaus - I simplified things a bit here because this method only needs to accept a list of package names (unlike uninstall which needs to accept a list of requirements files, etc.).

.path()
.parent()
.expect("package path is not root")
.simplified_display()
Copy link
Member

Choose a reason for hiding this comment

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

@ChannyClaus - I think pip show always shows the site-packages location regardless of whether it's editable. It looks like pip show has a separate Editable location field for the editable source, so we can add that in a separate PR?

Copy link
Contributor Author

@ChannyClaus ChannyClaus Mar 6, 2024

Choose a reason for hiding this comment

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

Ah, you're absolutely right, thanks for catching that - will look into that sometime, probably sometime this weekend.

// Print a separator between packages.
#[allow(clippy::print_stdout)]
{
println!("---");
Copy link
Member

Choose a reason for hiding this comment

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

I also had to change these back to using println, because the printer always prints to stderr, but I think it's important that this info goes to stdout, so we have to do it "manually" (by passing in quiet, etc.). Something we could try to improve in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I improved the situation a bit here: #2227

@charliermarsh charliermarsh enabled auto-merge (squash) March 6, 2024 03:04
@charliermarsh charliermarsh changed the title Add uv pip show Implement uv pip show Mar 6, 2024
@charliermarsh charliermarsh merged commit 395be44 into astral-sh:main Mar 6, 2024
7 checks passed
@charliermarsh
Copy link
Member

Thanks @ChannyClaus!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command line interface compatibility Compatibility with a specification or another tool enhancement New feature or improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants