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

Implement ToOwned instead of having a custom to_owned method #286

Closed
clarfonthey opened this issue Mar 4, 2017 · 7 comments
Closed

Implement ToOwned instead of having a custom to_owned method #286

clarfonthey opened this issue Mar 4, 2017 · 7 comments

Comments

@clarfonthey
Copy link
Contributor

No description provided.

@clarfonthey
Copy link
Contributor Author

Additionally, parse methods should go into FromStr impls.

@SimonSapin
Copy link
Member

I don’t really care, so I’m not gonna implement this myself. If someone wants to send a PR I’ll take it.

@clarfonthey
Copy link
Contributor Author

@SimonSapin I will do that in a bit. I think that it doesn't make sense to close an issue like this if it's desired but not prioritised; it's a decent first bug for someone who wants to submit a PR.

@SimonSapin
Copy link
Member

if it's desired

It’s not. I don’t see the point. I’m only not opposed to it enough to reject a PR if someone else wants it enough to make a PR.

@SimonSapin
Copy link
Member

I see that this is linked from an issue on Clippy, which tends to be very opinionated.

I’d be much less sympathetic accepting code that I’ll need to maintain if it’s based purely on an opinion of how things should be in principle, rather than on someone’s concrete use case who will then actually use that code.

@clarfonthey
Copy link
Contributor Author

A very simple use case is storing Hosts as HashMap keys; because Host<String> would implement Borrow<Host<&str>>, you could request to get Host::Domain("localhost") in a map that stores owned copies.

As far as the actual ToOwned implementation goes, it's not necessarily required but the amount of effort is really in the conversion, not actually having a ToOwned implementation itself. The real bulk come from the Borrow that comes with it.

It does have feasible use cases, but I figured I'd just open an issue now because I'm focused on something else at the moment and didn't want to forget about it. I'll offer a PR tomorrow. :)

@SimonSapin
Copy link
Member

Ah, mentioning HashMap is already a much better motivation than “because Clippy said so”. But does anyone ever use Host in hash map key, or is that a theoretical use case that was made up in a search of why ToOwned might be useful?

To some extent, the only reason I created the Host type is to carry the return value of Url::host. If the standard library had a generic Either enum I’d have used that.

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

No branches or pull requests

2 participants