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

feat(client): Allow modifying the connect destination #1569

Closed
wants to merge 1 commit into from

Conversation

kpcyrd
Copy link

@kpcyrd kpcyrd commented Jun 14, 2018

This resolves #1564.

As far as I can tell, this is a non breaking change that could be released in a 0.0.X upgrade. Please let me know if you prefer a different implementation. :)

@seanmonstar
Copy link
Member

Thanks for starting this up!

This may be the simple way forward, I just want to discuss the pros and cons. In this case, a user can construct a Uri themselves, and then easily set it. What happens if they did so without a scheme or authority? dest.set_uri(Uri::default() would set it to /. Is that a problem? Should other connectors be able to handle empty string for scheme and host?

A possible alternative is to take a string instead, fn set_uri(&mut self, s: &str) -> Result<(), hyper::Error>. This could require absolute URIs, and return an error otherwise. But maybe it's fine to make connectors have to deal with empty strings.

Thoughts?

cc @sfackler

@sfackler
Copy link
Contributor

The fact that there's a URI inside of the Destination is currently an implementation detail, so I'd expect just pub fn set_host(&mut self, host: &str) -> Result<(), Error> and pub fn set_port(&mut self, port: Option<u16>).

@seanmonstar
Copy link
Member

I like that. Is there a good use for having set_scheme as well?

@sfackler
Copy link
Contributor

I can't think of one off the top of my head, but it seems like we might as well be uniform with the setters.

@kpcyrd
Copy link
Author

kpcyrd commented Jun 16, 2018

There are no setters on Uri, if the api allows modifying parts of the Destination, should I try to build a new Uri with the value provided? Is there a way to ensure a Uri meets all requirements of a a valid destination?

@seanmonstar
Copy link
Member

You can convert the current URI into it's parts, and then you can parse the new one and insert it, and try to convert back into a URI. If it's illegal, that should return an error already.

@seanmonstar
Copy link
Member

I wrote the suggested changes up over in #1572.

@kpcyrd
Copy link
Author

kpcyrd commented Jun 18, 2018

@seanmonstar awesome, thanks! closing in favour of #1572

@kpcyrd kpcyrd closed this Jun 18, 2018
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.

Can't create or modify hyper::client::connect::Destination
3 participants