-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Support default-extras
config in the project interface.
#12965
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
base: main
Are you sure you want to change the base?
Conversation
e7bff9f
to
742dc99
Compare
Apologies for the delay, reviewing now! |
I need to go back and read the PEP for what they do and don't specify but I wanna call out this fact right away: the To be clear, I think you're probably right to do so, given what I know of extras, but I want to make sure we're going eyes wide open into that fact. With But "install only the extras for a package, and not the package" would generally be a nonsense thing to ask for, right? So we indeed don't want that? If so then At that point it's also a question of whether |
The impl otherwise looks reasonable (more detailed review of the main refactor in the other PR). |
#[test] | ||
fn sync_default_extras() -> Result<()> { |
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.
rad that you added all these tests, it would be nice to include tests for the other commands you added support to as well.
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.
yes I'll add them!
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.
should I add them for the default groups for those commands too? They weren't there for the other commands last time I checked
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.
I wouldn't complain if you did but no biggie if you don't.
Yes I think so.
Good point! I think we could skip adding a That said, the draft PEP 771 proposes the pip interface so that specifying an extra would implicitly disable default extras. If the PEP gets accepted, we will have to make some changes. 😅 |
742dc99
to
4078940
Compare
Ok reading the PEP more closely, notable details:
|
Just confirming that, yes, extras are purely additive whereas groups are designed to be sensical "without the package". So only installing a given extra (but not the base package) is probably not something we should support. |
The pep also links a draft pip implementation pypa/pip@main...wheelnext:pip:pep_771 |
4078940
to
7edb8b5
Compare
It's interesting because a lot of the PEP's semantics are for when talking about the package as a dependency, but the scope of this PR, by nature of it being a uv extension, means it has no(?) application to the package as a dependency. So it's really more like dependency-groups where it's purely a "developer of this project" experience improvement. Very interesting, I'm not sure if the CLI flags want to reproduce the behaviour of the PEP. Although really all the difference is that |
Yes, I think since many of these changes involve package metadata, the resolver will need to handle them. But if the PEP is accepted, I expect it will also influence local development workflows. For example, if a package defines
As far as I can tell, yes that seems to be the key difference for us. We could choose to handle this differently in the project interface though. |
We would do the same thing we did with our old non-standard support for dependency-groups, where we take both sets of fields and merge them intelligently, with the understanding that everyone will want to move to the standard system when it's available (the maintenance burden is minimal). |
Flagging "do we want Context: the (draft) PEP for default-extras specifies that If you want |
Summary
Depends on #12964. Closes #8607. Closes #13174.
Test Plan
cargo test
.Note: The draft PEP 771 uses the term
default-optional-dependency-keys
, but I've gone withdefault-extras
here, since something like--no-default-optional-dependency-keys
felt a bit too cumbersome to type. Let me know if we'd prefer sticking with the PEP’s naming instead.