Skip to content
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

Replace () error types in several functions. #515

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tmccombs
Copy link
Contributor

@tmccombs tmccombs commented Jul 15, 2019

This is an initial pass at #299.

It does not change ParseError to the more idiomatic Error name, or
change with_default_port's return type.

I'm also not 100% sure that the errors I returned are correct.


This change is Reviewable

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #517) made this pull request unmergeable. Please resolve the merge conflicts.

This is an initial pass at servo#299.

It does not change `ParseError` to the more idiomatic `Error` name, or
change `with_default_port`'s return type.
I'm not sure if the errors are appropriate enough
@opilar
Copy link

opilar commented Jul 19, 2019

@tmccombs seems like you forgot to update windows specific function: file_url_segments_to_pathbuf_windows.

@opilar
Copy link

opilar commented Jul 19, 2019

Remaining fixes:

diff --git a/src/lib.rs b/src/lib.rs
index f87fb02..6690e82 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -2408,7 +2408,7 @@ fn file_url_segments_to_pathbuf(
 fn file_url_segments_to_pathbuf(
     host: Option<&str>,
     segments: str::Split<char>,
-) -> Result<PathBuf, ()> {
+) -> Result<PathBuf, ParseError> {
     file_url_segments_to_pathbuf_windows(host, segments)
 }
 
@@ -2417,16 +2417,16 @@ fn file_url_segments_to_pathbuf(
 fn file_url_segments_to_pathbuf_windows(
     host: Option<&str>,
     mut segments: str::Split<char>,
-) -> Result<PathBuf, ()> {
+) -> Result<PathBuf, ParseError> {
     let mut string = if let Some(host) = host {
         r"\\".to_owned() + host
     } else {
-        let first = segments.next().ok_or(())?;
+        let first = segments.next().ok_or(ParseError::InvalidLocalPath)?;
 
         match first.len() {
             2 => {
                 if !first.starts_with(parser::ascii_alpha) || first.as_bytes()[1] != b':' {
-                    return Err(());
+                    return Err(ParseError::InvalidLocalPath);
                 }
 
                 first.to_owned()
@@ -2434,17 +2434,17 @@ fn file_url_segments_to_pathbuf_windows(
 
             4 => {
                 if !first.starts_with(parser::ascii_alpha) {
-                    return Err(());
+                    return Err(ParseError::InvalidLocalPath);
                 }
                 let bytes = first.as_bytes();
                 if bytes[1] != b'%' || bytes[2] != b'3' || (bytes[3] != b'a' && bytes[3] != b'A') {
-                    return Err(());
+                    return Err(ParseError::InvalidLocalPath);
                 }
 
                 first[0..1].to_owned() + ":"
             }
 
-            _ => return Err(()),
+            _ => return Err(ParseError::InvalidLocalPath),
         }
     };
 
@@ -2454,7 +2454,7 @@ fn file_url_segments_to_pathbuf_windows(
         // Currently non-unicode windows paths cannot be represented
         match String::from_utf8(percent_decode(segment.as_bytes()).collect()) {
             Ok(s) => string.push_str(&s),
-            Err(..) => return Err(()),
+            Err(..) => return Err(ParseError::InvalidLocalPath),
         }
     }
     let path = PathBuf::from(string); 

@djc
Copy link
Contributor

djc commented Aug 19, 2020

Changing the title to note that these changes are semver-incompatible (in my understanding). Tracking all proposed semver-incompatible changes in #627.

@djc djc changed the title Replace () error types in several functions. [SemVer-incompatible] Replace () error types in several functions. Aug 19, 2020
@djc djc changed the title [SemVer-incompatible] Replace () error types in several functions. Replace () error types in several functions. Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants