Replace () error types with specific types that implement Error

[brson]

There are a number of methods in url that return Result<_, ()>. () though does not implement Error and so does not inter operate cleanly with callers that want to treat the result as Error (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.

[dtolnay]

Yep we struggled with this in #323. It makes it hard to handle these errors concisely.

[Enet4]

Here's a list of public methods returning () as an error:

1. Url#path_segments_mut
2. Url#set_port
3. Url#set_ip_host
4. Url#set_password
5. Url#set_username
6. Url#set_scheme
7. Url#from_file_path
8. Url#from_directory_path
9. 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.

[tmccombs]

I'll start working on this.

[nox]

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)

1 to 6 don't exactly have the same error conditions, actually.

• 2 wants an non-file: URL with a host.
• 4-6 want a URL with a host.

[nox]

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.

[tmccombs]

In my PR I went with EmptyHost for 4-6. But would also be ok with making a new error type for them.

[bstrie]

@tmccombs Are you pursuing this? If not, I might be interested in resurrecting your patch.

[tmccombs]

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.

[bstrie]

@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.

[djc]

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.

[hoijui]

This is also an issue when using the thiserror create, as one can not easily wrapp () with thiserrors #[from].

[mhnap]

Maybe there are some updates on this issue? Or some help is needed to fix it?