Move modification

astral-sh · Mar 18, 2024 · 0f10760 · 0f10760
1 parent 9bded81
commit 0f10760
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 53 deletions.
diff --git a/crates/pep508-rs/src/lib.rs b/crates/pep508-rs/src/lib.rs
@@ -1755,7 +1755,7 @@ mod tests {
         } 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 @ C:/Users/ferris/wheel-0.42.0.whl#hash=somehash",
             ]
         };
 
@@ -1765,13 +1765,8 @@ mod tests {
                 .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("")
-                )));
+                assert!(url.path().ends_with("/Users/ferris/wheel-0.42.0.whl"));
             }
         }
     }

diff --git 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<Path>) -> 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,53 +65,55 @@ 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<Path>, working_dir: impl AsRef<Path>) -> 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)
         };
 
         // 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 }
     }
 
     /// Parse a URL from an absolute path.
     pub fn parse_absolute_path(path: impl AsRef<Path>) -> Result<Self, VerbatimUrlError> {
-        // 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<Path> + '_, 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<Path>, 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)
+        );
     }
 }