Skip to content

Commit

Permalink
Use filename trait for WheelWire conversion (astral-sh#3651)
Browse files Browse the repository at this point in the history
## Summary

The main motivation here is that the `.filename()` method that we
implement on `Url` 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 in `failed to extract filename
from URL: {url}`, so in theory we want the underlying errors to _omit_
the URL? But sometimes they use `#[error(transparent)]`?
  • Loading branch information
charliermarsh authored May 20, 2024
1 parent 657eebd commit f3965fe
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 15 deletions.
7 changes: 5 additions & 2 deletions crates/distribution-types/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,11 @@ pub enum Error {
#[error(transparent)]
WheelFilename(#[from] distribution_filename::WheelFilenameError),

#[error("Unable to extract filename from URL: {0}")]
UrlFilename(Url),
#[error("Unable to extract file path from URL: {0}")]
MissingFilePath(Url),

#[error("Could not extract path segments from URL: {0}")]
MissingPathSegments(Url),

#[error("Distribution not found at: {0}")]
NotFound(Url),
Expand Down
10 changes: 6 additions & 4 deletions crates/distribution-types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -709,13 +709,15 @@ impl RemoteSource for File {
impl RemoteSource for Url {
fn filename(&self) -> Result<Cow<'_, str>, Error> {
// Identify the last segment of the URL as the filename.
let filename = self
let path_segments = self
.path_segments()
.and_then(Iterator::last)
.ok_or_else(|| Error::UrlFilename(self.clone()))?;
.ok_or_else(|| Error::MissingPathSegments(self.clone()))?;

// This is guaranteed by the contract of `Url::path_segments`.
let last = path_segments.last().expect("path segments is non-empty");

// Decode the filename, which may be percent-encoded.
let filename = urlencoding::decode(filename)?;
let filename = urlencoding::decode(last)?;

Ok(filename)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-installer/src/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ impl<'a> Planner<'a> {
// Store the canonicalized path, which also serves to validate that it exists.
let path = match url
.to_file_path()
.map_err(|()| Error::UrlFilename(url.to_url()))?
.map_err(|()| Error::MissingFilePath(url.to_url()))?
.canonicalize()
{
Ok(path) => path,
Expand Down
15 changes: 7 additions & 8 deletions crates/uv-resolver/src/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1087,15 +1087,14 @@ impl TryFrom<WheelWire> for Wheel {
type Error = String;

fn try_from(wire: WheelWire) -> Result<Wheel, String> {
let path_segments = wire
.url
.path_segments()
.ok_or_else(|| format!("could not extract path from URL `{}`", wire.url))?;
// This is guaranteed by the contract of Url::path_segments.
let last = path_segments.last().expect("path segments is non-empty");
let filename = last
// Extract the filename segment from the URL.
let filename = wire.url.filename().map_err(|err| err.to_string())?;

// Parse the filename as a wheel filename.
let filename = filename
.parse::<WheelFilename>()
.map_err(|err| format!("failed to parse `{last}` as wheel filename: {err}"))?;
.map_err(|err| format!("failed to parse `{filename}` as wheel filename: {err}"))?;

Ok(Wheel {
url: wire.url,
hash: wire.hash,
Expand Down

0 comments on commit f3965fe

Please sign in to comment.