From 9bded81b9f3412833b88347268b6ca8ed315feeb Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Mon, 18 Mar 2024 08:52:53 +0800 Subject: [PATCH 1/2] Allow path with fragment --- crates/pep508-rs/src/lib.rs | 31 +++++++++++++ crates/pep508-rs/src/verbatim_url.rs | 65 ++++++++++++++++++++++++++-- 2 files changed, 93 insertions(+), 3 deletions(-) diff --git a/crates/pep508-rs/src/lib.rs b/crates/pep508-rs/src/lib.rs index 9b05d72c9e9a..f96c57d77acf 100644 --- a/crates/pep508-rs/src/lib.rs +++ b/crates/pep508-rs/src/lib.rs @@ -1744,4 +1744,35 @@ mod tests { let requirement = Requirement::from_str("pytest;'4.0'>=python_version").unwrap(); assert_eq!(requirement.to_string(), "pytest ; '4.0' >= python_version"); } + + #[test] + fn path_with_fragment() { + let requirements = if cfg![not(windows)] { + &[ + "wheel @ file:///Users/ferris/wheel-0.42.0.whl#hash=somehash", + "wheel @ /Users/ferris/wheel-0.42.0.whl#hash=somehash", + ] + } else { + &[ + "wheel @ file:///C:/Users/ferris/wheel-0.42.0.whl#hash=somehash", + "wheel @ /C:/Users/ferris/wheel-0.42.0.whl#hash=somehash", + ] + }; + + for requirement in requirements { + if let VersionOrUrl::Url(url) = Requirement::from_str(requirement) + .unwrap() + .version_or_url + .unwrap() + { + assert!(url.path().ends_with("/Users/ferris/wheel-0.42.0.whl")); + assert_eq!(url.fragment(), Some("hash=somehash")); + assert!(url.given().unwrap().ends_with(&format!( + "{}#{}", + url.path(), + url.fragment().unwrap_or("") + ))); + } + } + } } diff --git a/crates/pep508-rs/src/verbatim_url.rs b/crates/pep508-rs/src/verbatim_url.rs index 7e6559d8794a..3021ae788d63 100644 --- a/crates/pep508-rs/src/verbatim_url.rs +++ b/crates/pep508-rs/src/verbatim_url.rs @@ -37,8 +37,17 @@ impl VerbatimUrl { /// Create a [`VerbatimUrl`] from a file path. pub fn from_path(path: impl AsRef) -> Self { + // Split fragment from path + let path_str = path.as_ref().to_string_lossy(); + let (path, fragment) = split_fragment(path_str.as_ref()); + let path = normalize_path(path.as_ref()); - let url = Url::from_file_path(path).expect("path is absolute"); + let mut url = Url::from_file_path(path).expect("path is absolute"); + + // Set fragment for file url if exists + if fragment.is_some() { + url.set_fragment(fragment); + } Self { url, given: None } } @@ -51,6 +60,10 @@ impl VerbatimUrl { /// Parse a URL from an absolute or relative path. #[cfg(feature = "non-pep508-extensions")] // PEP 508 arguably only allows absolute file URLs. pub fn parse_path(path: impl AsRef, working_dir: impl AsRef) -> Self { + // Split fragment from path + let path_str = path.as_ref().to_string_lossy(); + let (path, fragment) = split_fragment(path_str.as_ref()); + // Convert the path to an absolute path, if necessary. let path = if path.as_ref().is_absolute() { path.as_ref().to_path_buf() @@ -62,13 +75,22 @@ impl VerbatimUrl { let path = normalize_path(path); // Convert to a URL. - let url = Url::from_file_path(path).expect("path is absolute"); + let mut url = Url::from_file_path(path).expect("path is absolute"); + + // Set fragment for file url if exists + if fragment.is_some() { + url.set_fragment(fragment); + } Self { url, given: None } } /// Parse a URL from an absolute path. pub fn parse_absolute_path(path: impl AsRef) -> Result { + // Split fragment from path + let path_str = path.as_ref().to_string_lossy(); + let (path, fragment) = split_fragment(path_str.as_ref()); + // Convert the path to an absolute path, if necessary. let path = if path.as_ref().is_absolute() { path.as_ref().to_path_buf() @@ -80,7 +102,12 @@ impl VerbatimUrl { let path = normalize_path(path); // Convert to a URL. - let url = Url::from_file_path(path).expect("path is absolute"); + let mut url = Url::from_file_path(path).expect("path is absolute"); + + // Set fragment for file url if exists + if fragment.is_some() { + url.set_fragment(fragment); + } Ok(Self { url, given: None }) } @@ -222,6 +249,12 @@ pub fn split_scheme(s: &str) -> Option<(&str, &str)> { Some((scheme, rest)) } +// split fragment from string +fn split_fragment(s: &str) -> (impl AsRef + '_, Option<&str>) { + s.split_once('#') + .map_or_else(|| (s, None), |(path, fragment)| (path, Some(fragment))) +} + /// A supported URL scheme for PEP 508 direct-URL requirements. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Scheme { @@ -363,4 +396,30 @@ mod tests { ); assert_eq!(split_scheme("https:"), Some(("https", ""))); } + + #[test] + fn fragment() { + for (s, expected) in [ + ( + "file:///home/ferris/project/scripts#hash=somehash", + ("file:///home/ferris/project/scripts", Some("hash=somehash")), + ), + ( + "file:home/ferris/project/scripts#hash=somehash", + ("file:home/ferris/project/scripts", Some("hash=somehash")), + ), + ( + "/home/ferris/project/scripts#hash=somehash", + ("/home/ferris/project/scripts", Some("hash=somehash")), + ), + ( + "file:///home/ferris/project/scripts", + ("file:///home/ferris/project/scripts", None), + ), + ("", ("", None)), + ] { + let (path, frag) = split_fragment(s); + assert_eq!((path.as_ref(), frag), (Path::new(expected.0), expected.1)); + } + } } From 131b1454265d7c522c7de0cb9c09dbcaf41dc3cb Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 18 Mar 2024 12:34:22 -0400 Subject: [PATCH 2/2] Move modification --- crates/pep508-rs/src/lib.rs | 42 +++++----- crates/pep508-rs/src/verbatim_url.rs | 121 +++++++++++++++++---------- 2 files changed, 97 insertions(+), 66 deletions(-) diff --git a/crates/pep508-rs/src/lib.rs b/crates/pep508-rs/src/lib.rs index f96c57d77acf..89c70c7cc73c 100644 --- a/crates/pep508-rs/src/lib.rs +++ b/crates/pep508-rs/src/lib.rs @@ -1128,7 +1128,7 @@ mod tests { parse_markers_impl, MarkerExpression, MarkerOperator, MarkerTree, MarkerValue, MarkerValueString, MarkerValueVersion, }; - use crate::{Cursor, Requirement, VerbatimUrl, VersionOrUrl}; + use crate::{Cursor, Pep508Error, Requirement, VerbatimUrl, VersionOrUrl}; fn parse_err(input: &str) -> String { Requirement::from_str(input).unwrap_err().to_string() @@ -1746,33 +1746,35 @@ mod tests { } #[test] - fn path_with_fragment() { - let requirements = if cfg![not(windows)] { + fn path_with_fragment() -> Result<(), Pep508Error> { + let requirements = if cfg!(windows) { &[ - "wheel @ file:///Users/ferris/wheel-0.42.0.whl#hash=somehash", - "wheel @ /Users/ferris/wheel-0.42.0.whl#hash=somehash", + "wheel @ file:///C:/Users/ferris/wheel-0.42.0.whl#hash=somehash", + "wheel @ C:/Users/ferris/wheel-0.42.0.whl#hash=somehash", ] } else { &[ - "wheel @ file:///C:/Users/ferris/wheel-0.42.0.whl#hash=somehash", - "wheel @ /C:/Users/ferris/wheel-0.42.0.whl#hash=somehash", + "wheel @ file:///Users/ferris/wheel-0.42.0.whl#hash=somehash", + "wheel @ /Users/ferris/wheel-0.42.0.whl#hash=somehash", ] }; for requirement in requirements { - if let VersionOrUrl::Url(url) = Requirement::from_str(requirement) - .unwrap() - .version_or_url - .unwrap() - { - assert!(url.path().ends_with("/Users/ferris/wheel-0.42.0.whl")); - assert_eq!(url.fragment(), Some("hash=somehash")); - assert!(url.given().unwrap().ends_with(&format!( - "{}#{}", - url.path(), - url.fragment().unwrap_or("") - ))); - } + // Extract the URL. + let Some(VersionOrUrl::Url(url)) = Requirement::from_str(requirement)?.version_or_url + else { + unreachable!("Expected a URL") + }; + + // Assert that the fragment and path have been separated correctly. + assert_eq!(url.fragment(), Some("hash=somehash")); + assert!( + url.path().ends_with("/Users/ferris/wheel-0.42.0.whl"), + "Expected the path to end with '/Users/ferris/wheel-0.42.0.whl', found '{}'", + url.path() + ); } + + Ok(()) } } diff --git a/crates/pep508-rs/src/verbatim_url.rs b/crates/pep508-rs/src/verbatim_url.rs index 3021ae788d63..62f3907aa08f 100644 --- a/crates/pep508-rs/src/verbatim_url.rs +++ b/crates/pep508-rs/src/verbatim_url.rs @@ -37,17 +37,22 @@ impl VerbatimUrl { /// Create a [`VerbatimUrl`] from a file path. pub fn from_path(path: impl AsRef) -> Self { - // Split fragment from path - let path_str = path.as_ref().to_string_lossy(); - let (path, fragment) = split_fragment(path_str.as_ref()); + let path = path.as_ref(); - let path = normalize_path(path.as_ref()); + // Normalize the path. + let path = normalize_path(path); + + // Extract the fragment, if it exists. + let (path, fragment) = split_fragment(&path); + + // Convert to a URL. let mut url = Url::from_file_path(path).expect("path is absolute"); - // Set fragment for file url if exists - if fragment.is_some() { - url.set_fragment(fragment); + // Set the fragment, if it exists. + if let Some(fragment) = fragment { + url.set_fragment(Some(fragment)); } + Self { url, given: None } } @@ -60,13 +65,11 @@ impl VerbatimUrl { /// Parse a URL from an absolute or relative path. #[cfg(feature = "non-pep508-extensions")] // PEP 508 arguably only allows absolute file URLs. pub fn parse_path(path: impl AsRef, working_dir: impl AsRef) -> Self { - // Split fragment from path - let path_str = path.as_ref().to_string_lossy(); - let (path, fragment) = split_fragment(path_str.as_ref()); + let path = path.as_ref(); // Convert the path to an absolute path, if necessary. - let path = if path.as_ref().is_absolute() { - path.as_ref().to_path_buf() + let path = if path.is_absolute() { + path.to_path_buf() } else { working_dir.as_ref().join(path) }; @@ -74,12 +77,15 @@ impl VerbatimUrl { // Normalize the path. let path = normalize_path(path); + // Extract the fragment, if it exists. + let (path, fragment) = split_fragment(&path); + // Convert to a URL. let mut url = Url::from_file_path(path).expect("path is absolute"); - // Set fragment for file url if exists - if fragment.is_some() { - url.set_fragment(fragment); + // Set the fragment, if it exists. + if let Some(fragment) = fragment { + url.set_fragment(Some(fragment)); } Self { url, given: None } @@ -87,26 +93,27 @@ impl VerbatimUrl { /// Parse a URL from an absolute path. pub fn parse_absolute_path(path: impl AsRef) -> Result { - // Split fragment from path - let path_str = path.as_ref().to_string_lossy(); - let (path, fragment) = split_fragment(path_str.as_ref()); + let path = path.as_ref(); // Convert the path to an absolute path, if necessary. - let path = if path.as_ref().is_absolute() { - path.as_ref().to_path_buf() + let path = if path.is_absolute() { + path.to_path_buf() } else { - return Err(VerbatimUrlError::RelativePath(path.as_ref().to_path_buf())); + return Err(VerbatimUrlError::RelativePath(path.to_path_buf())); }; // Normalize the path. let path = normalize_path(path); + // Extract the fragment, if it exists. + let (path, fragment) = split_fragment(&path); + // Convert to a URL. let mut url = Url::from_file_path(path).expect("path is absolute"); - // Set fragment for file url if exists - if fragment.is_some() { - url.set_fragment(fragment); + // Set the fragment, if it exists. + if let Some(fragment) = fragment { + url.set_fragment(Some(fragment)); } Ok(Self { url, given: None }) @@ -249,10 +256,20 @@ pub fn split_scheme(s: &str) -> Option<(&str, &str)> { Some((scheme, rest)) } -// split fragment from string -fn split_fragment(s: &str) -> (impl AsRef + '_, Option<&str>) { - s.split_once('#') - .map_or_else(|| (s, None), |(path, fragment)| (path, Some(fragment))) +/// Split the fragment from a URL. +/// +/// For example, given `file:///home/ferris/project/scripts#hash=somehash`, returns +/// `("/home/ferris/project/scripts", Some("hash=somehash"))`. +fn split_fragment(path: &Path) -> (Cow, Option<&str>) { + let Some(s) = path.to_str() else { + return (Cow::Borrowed(path), None); + }; + + let Some((path, fragment)) = s.split_once('#') else { + return (Cow::Borrowed(path), None); + }; + + (Cow::Owned(PathBuf::from(path)), Some(fragment)) } /// A supported URL scheme for PEP 508 direct-URL requirements. @@ -399,27 +416,39 @@ mod tests { #[test] fn fragment() { - for (s, expected) in [ + assert_eq!( + split_fragment(Path::new( + "file:///home/ferris/project/scripts#hash=somehash" + )), ( - "file:///home/ferris/project/scripts#hash=somehash", - ("file:///home/ferris/project/scripts", Some("hash=somehash")), - ), + Cow::Owned(PathBuf::from("file:///home/ferris/project/scripts")), + Some("hash=somehash") + ) + ); + assert_eq!( + split_fragment(Path::new("file:home/ferris/project/scripts#hash=somehash")), ( - "file:home/ferris/project/scripts#hash=somehash", - ("file:home/ferris/project/scripts", Some("hash=somehash")), - ), + Cow::Owned(PathBuf::from("file:home/ferris/project/scripts")), + Some("hash=somehash") + ) + ); + assert_eq!( + split_fragment(Path::new("/home/ferris/project/scripts#hash=somehash")), ( - "/home/ferris/project/scripts#hash=somehash", - ("/home/ferris/project/scripts", Some("hash=somehash")), - ), + Cow::Owned(PathBuf::from("/home/ferris/project/scripts")), + Some("hash=somehash") + ) + ); + assert_eq!( + split_fragment(Path::new("file:///home/ferris/project/scripts")), ( - "file:///home/ferris/project/scripts", - ("file:///home/ferris/project/scripts", None), - ), - ("", ("", None)), - ] { - let (path, frag) = split_fragment(s); - assert_eq!((path.as_ref(), frag), (Path::new(expected.0), expected.1)); - } + Cow::Borrowed(Path::new("file:///home/ferris/project/scripts")), + None + ) + ); + assert_eq!( + split_fragment(Path::new("")), + (Cow::Borrowed(Path::new("")), None) + ); } }