Skip to content

feat(*): make all errors cloneable#128

Closed
srijs wants to merge 1 commit into
hyperium:masterfrom
srijs:feat/all-errors-cloneable
Closed

feat(*): make all errors cloneable#128
srijs wants to merge 1 commit into
hyperium:masterfrom
srijs:feat/all-errors-cloneable

Conversation

@srijs

@srijs srijs commented Sep 13, 2017

Copy link
Copy Markdown

When working on implementing compatibility with the http crate over in hyper, I ran into a problem where the error types in http are not Clone. This PR changes that.

I guess this locks in the error API quite a bit (errors can't contain any non-cloneable information), but on the other hand it also makes it possible for hyper to leverage logic from http more easily.

Let me know if you like this or if I should try to make do with non-cloneable errors.

@alexcrichton

Copy link
Copy Markdown
Contributor

The main error I know of that's not cloneable is io::Error, and that's mostly for trait-object reasons where it can contain any other error. I've found because of that cloneable errors tend to not come up much in practice, but you found it would be useful in this case?

@srijs

srijs commented Sep 13, 2017

Copy link
Copy Markdown
Author

My main use-case for it so far has been that Hyper's UriError is cloneable, and so it would aid compatibility for the http::uri::InvalidUri* errors to be cloneable as well.

As to how it's useful for hyper::UriError to impl Clone in the first place, maybe @seanmonstar can provide more insight?

@alexcrichton

Copy link
Copy Markdown
Contributor

Aha that'd do it! I wonder if maybe the concrete error types here could be cloneable (the ones for each specific situation) but then the "global" http::error::Error type could be non-cloneable?

@carllerche

Copy link
Copy Markdown
Collaborator

I agree that http::Error should not be clone. The concrete error types could.

Also, a nit, but this repo isn't using the angular style commit messages :)

@seanmonstar

Copy link
Copy Markdown
Member

The trend in the ecosystem is for errors to not implement Clone. It's likely safer for backwards compatibility if these errors don't implement Clone, otherwise it can restrict what things we can put inside the error types.

A benefit to this in my mind is that, with errors that cannot be construct from outside the crate, that means that you only ever have an error when the crate detected one. If you can clone them, it's possible to think that more errors occurred than really did.

As to how it's useful for hyper::UriError to impl Clone in the first place, maybe @seanmonstar can provide more insight?

That sounds like a mistake :D

@srijs

srijs commented Sep 17, 2017

Copy link
Copy Markdown
Author

Thanks for the input, y'all. Based on the ecosystem trends you pointed out, as well as the direction my compatibility PR for hyper is moving, I think it makes sense to just close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants