Open brson opened 7 years ago
Yep we struggled with this in #323. It makes it hard to handle these errors concisely.
Here's a list of public methods returning ()
as an error:
Url#path_segments_mut
Url#set_port
Url#set_ip_host
Url#set_password
Url#set_username
Url#set_scheme
Url#from_file_path
Url#from_directory_path
Url#to_file_path
Although Url#path_segments
returns an Option
, this is not very consistent when we look at its "_mut" counterpart path_segments_mut
, which returns a Result
.
I would have suggested a CannotBeBase
variant of ParseError
for all methods requiring a URL that is not cannot-be-base (this includes methods 1 to 6 above), but it seems that the current design is to have one for each method. Should we change that to make the error enum more concise? I cannot think of a good reason to distinguish SetHostCannotBeBase
from PathSegmentsCannotBeBase
or SetSchemeCannotBeBase
.
Methods 7 and 8 could return a new variant ParseError::PathNotAbsolute
. Method 9 could yield a new ParseError::InvalidLocalPath
.
Let's not also forget Url#with_default_port
, which takes an argument of type FnOnce() -> Result<u16, ()>
. Once we harmonize all the results, we might be able to just replace the error type with ParseError
.
I'll start working on this.
I would have suggested a
CannotBeBase
variant ofParseError
for all methods requiring a URL that is not cannot-be-base (this includes methods 1 to 6 above)
1 to 6 don't exactly have the same error conditions, actually.
file:
URL with a host.I guess for 4-6 we could return EmptyHost
, but I am a bit weirded out by the self.host() == Some(Host::Domain(""))
test in set_password
which is not found in the others.
In my PR I went with EmptyHost
for 4-6. But would also be ok with making a new error type for them.
@tmccombs Are you pursuing this? If not, I might be interested in resurrecting your patch.
tbh, I'm not entirely sure what to do with this, since I missed the last major release and this is a backwards incompatible change.
@tmccombs I have an idea for a way to make this happen that wouldn't involve a breaking change, I'll take a crack at it based on your PR.
As I understand it, this change would be semver-incompatible. I've opened a tracking issue in #627 for proposed semver-incompatible changes to discuss how these could be handled going forward.
This is also an issue when using the thiserror
create, as one can not easily wrapp ()
with thiserror
s #[from]
.
Maybe there are some updates on this issue? Or some help is needed to fix it?
There are a number of methods in url that return
Result<_, ()>
.()
though does not implementError
and so does not inter operate cleanly with callers that want to treat the result asError
(like error-chain).In today's Rust the best pattern for this is probably to create a single
Error
enum for the entire crate to share and return it everywhere.Would require a major version bump.