feat: error on non-existent extra from lock file#10925
feat: error on non-existent extra from lock file#10925mkniewallner wants to merge 4 commits intoastral-sh:gankra/lock-extrafrom
Conversation
| #[error("Extra `{0}` is not defined in the project's `optional-dependencies` table")] | ||
| MissingExtraProject(ExtraName), | ||
|
|
||
| #[error("Extra `{0}` is not defined in any project's `optional-dependencies` table")] | ||
| MissingExtraWorkspace(ExtraName), |
There was a problem hiding this comment.
Wording could probably be adapted here, as the check happens at lock file level, but not sure exactly how we would want to phrase this.
mkniewallner
force-pushed
the
feat/error-on-non-existent-extra-from-lock
branch
from
9121e63 to
d34e9ad
Compare
|
Oh rad, thanks for the rewrite/rebase! (We'll land the base branch this week at which point this can land too) |
Gankra
force-pushed
the
gankra/lock-extra
branch
from
11d6ebd to
487d5f5
Compare
Gankra
force-pushed
the
feat/error-on-non-existent-extra-from-lock
branch
from
d34e9ad to
339255e
Compare
|
forehead slap What a terrible github behaviour. I can't even reopen this! Sorry! (It needs to be retargetted to tracking/060) |
|
In the meantime I'll still review this... |
No worries, will rebase and recreate the PR :) |
Gankra
left a comment
Gankra
left a comment
There was a problem hiding this comment.
Basic idea looks right but needs to handle a corner case.
| /// Returns `true` if the package contains the [`ExtraName`]. | ||
| pub fn contains_extra(&self, name: &ExtraName) -> bool { | ||
| self.metadata | ||
| .provides_extras | ||
| .as_ref() | ||
| .is_some_and(|provides_extras| provides_extras.contains(name)) | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is the right place to handle this but I believe this isn't our desired semantic for the case where provides_extras is undefined. We aren't doing a lockfile version bump, so we're still supporting provides_extras being missing.
Some part of this code should handle that as "I don't actually have information on if the extra is defined, and will conservatively not error, because I'm uncertain".
There was a problem hiding this comment.
Oh yeah definitely, will update the logic.
## Summary Closes #10597. Recreated #10925 that got closed as the base branch got merged. ## Test Plan Snapshot tests. --------- Co-authored-by: Aria Desires <[email protected]>
## Summary Closes #10597. Recreated #10925 that got closed as the base branch got merged. ## Test Plan Snapshot tests. --------- Co-authored-by: Aria Desires <[email protected]>
Closes #10597. Recreated #10925 that got closed as the base branch got merged. Snapshot tests. --------- Co-authored-by: Aria Desires <[email protected]>
Closes #10597. Recreated #10925 that got closed as the base branch got merged. Snapshot tests. --------- Co-authored-by: Aria Desires <[email protected]>
Closes #10597. Recreated #10925 that got closed as the base branch got merged. Snapshot tests. --------- Co-authored-by: Aria Desires <[email protected]>
Closes astral-sh#10597. Recreated astral-sh#10925 that got closed as the base branch got merged. Snapshot tests. --------- Co-authored-by: Aria Desires <[email protected]>
2 participants
Summary
Based on #11063.
Alternative to #10869 which follows the suggestion, in #10869 (comment), to validate extras from the lock file instead of
pyproject.toml.Test Plan
Snapshot tests.
Some tests are failing, but it seems unrelated to the changes, and likely due to the branch being based on an outdated branch (rebasing the branch from latest
mainmakes one of the failing test pass locally).