-
Notifications
You must be signed in to change notification settings - Fork 346
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
Use filename trait for WheelWire
conversion
#3651
Conversation
.ok_or_else(|| Error::MissingPathSegments)?; | ||
|
||
// This is guaranteed by the contract of `Url::path_segments`. | ||
let last = path_segments.last().expect("path segments is non-empty"); |
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.
Ported this over from your version in lock.rs
.
#[error("Unable to extract file path from URL: {0}")] | ||
MissingFilePath(Url), | ||
|
||
#[error("Could not extract path segments")] |
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.
Weird that this omits the URL, but MissingFilePath
includes it, because the clients of these errors sometimes include the URL and sometimes don't. Specifically, where we return MissingFilePath
, we need to include the URL; but where we return MissingPathSegments
, we want to exclude it, because we wrap it in a message that includes the URL (so including it here would duplicate it). I don't know what the right strategy is.
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.
Okay, changed these to both include the URL, and removed the "wrap the error in failed to extract filename from URL
" behavior from lock.rs
.
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.
Makes sense to me!
Summary
The main motivation here is that the
.filename()
method that we implement onUrl
will do URL decoding for the last segment, which we were missing here.The errors are a bit awkward, because in
crates/uv-resolver/src/lock.rs
, we wrap infailed to extract filename from URL: {url}
, so in theory we want the underlying errors to omit the URL? But sometimes they use#[error(transparent)]
?