Skip to content

Return Result<{Self,&Self,&mut Self}, _> instead of Result<(), _> to allow method chaining #375

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

Open
StaloneLab opened this issue Jul 4, 2017 · 6 comments
Assignees

Comments

@StaloneLab
Copy link

StaloneLab commented Jul 4, 2017

I'm working on the issue #299 right now and I saw some methods return Result<(), ()>, the issue I mentionned is about making them return a ParseResult<()>, but wouldn't it be better if they returned ParseResult<Self>, in order to allow method chaining with ? or (old) unwrap()?

This would actually be useful mainly for setters, I set up an actual list of them:

  1. #set_href
  2. HostInternal#serialize<S> (part of serde crate, but I think can be replaced)
  3. (IDNA) #domain_to_unicode
  4. Url#set_port
  5. Url#set_ip_host
  6. Url#set_password
  7. Url#set_username
  8. Url#set_scheme

Could work on this, be I need to be validated by someone before.

@SimonSapin
Copy link
Member

Is this different from #299 ?

I’d prefer not to make breaking changes until there’s a stronger need to do so.

@StaloneLab
Copy link
Author

StaloneLab commented Jul 4, 2017

Not for Errors here, but for Ok().
That's what I was saying just over, issue #299 suggest to return Result<(), SomeError> instead of Result<(), ()>, I'm stating Result<Self, SomeError> would be better.

I’d prefer not to make breaking changes until there’s a stronger need to do so.

Any reason for that?

@SimonSapin
Copy link
Member

() as error is a big nono, though, isn't it?

Thank you for your well-argumented constructive input to this discussion.

@SimonSapin
Copy link
Member

SimonSapin commented Jul 15, 2017

I’d prefer not to make breaking changes until there’s a stronger need to do so.

Any reason for that?

To be able to url 2.0 in Servo we’d first need to upgrade Hyper. That would be a breaking change in Hyper since Url is part of its public API. Then we’ll also need to do the same for each crate that depends on Hyper, then for crates that depend on those… There’s a cascading effect.

It’d involve coordinating pull requests to dozens of projects, with almost as many maintainers. It’d be a big pain.

@SimonSapin SimonSapin changed the title Returning coherent data for setters Return Result<{Self,&Self,&mut Self}, _> instead of Result<(), _> to allow method chaining Oct 17, 2018
@nox nox self-assigned this Jul 18, 2019
@bstrie
Copy link

bstrie commented Oct 31, 2019

Considering that Hyper (along with everyone else in the web domain) is at this moment preparing for a breaking change due to async/await stabilization, if ever there was a time to consider such a breaking change it would be now.

@jdm
Copy link
Member

jdm commented Oct 31, 2019

Hyper no longer depends on rust-url. That being said, there was recently a breaking major release of rust-url, and it was, indeed, a big pain.

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

No branches or pull requests

5 participants