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 FromStr for Url #107

Merged
merged 1 commit into from May 10, 2015
Merged

implement FromStr for Url #107

merged 1 commit into from May 10, 2015

Conversation

@blaenk
Copy link
Contributor

blaenk commented May 9, 2015

This enables the use of the parse() method on &str

Fixes #18.

This enables the use of the `parse()` method on `&str`
@seanmonstar
Copy link
Contributor

seanmonstar commented May 9, 2015

Just warning, this has been closed as wontfix a few times now.

On Sat, May 9, 2015, 4:02 PM Jorge Israel Peña notifications@github.com
wrote:

This enables the use of the parse() method on &str

You can view, comment on, or merge this pull request online at:

#107
Commit Summary

  • implement FromStr for Url

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#107.

@blaenk
Copy link
Contributor Author

blaenk commented May 9, 2015

Thanks for the heads up, I guess I should've searched, but I didn't think it would be particularly controversial. I've contributed the same change to the rust-lang/regex and rust-lang/glob packages and they were merged.

@blaenk
Copy link
Contributor Author

blaenk commented May 9, 2015

From @SimonSapin in #18:

I’ve resisted doing this so far because I really don’t like FromStr. It feels like "too much magic" to me: when reading code that uses it, I sometimes have no idea what the expected type is because the compiler infers it.

Back then it was typically used with from_str, but nowadays you have the parse method which is a common interface for parsing all kinds of types, and is covered pretty early on in the rust book. Yeah the type is still inferred, but I don't think it's all that confusing when you have something like "thing".parse::<Type>() or let ty: Type = "thing".parse().unwrap(). Another difference from the time that was written is that parse() now yields a Result, so you don't lose error information when the parsing fails.

rust-lang/regex and rust-lang/glob also have impls for this trait, and I've seen it in many other packages as well, such as the toml package. It's a nice, uniform way of parsing strings, especially for types like these that map very readily to existing methods.

@SimonSapin
Copy link
Member

SimonSapin commented May 10, 2015

I’m still not a huge fan of FromStr, but I suppose it doesn’t hurt much to implement it.

Thanks for the patch!

SimonSapin added a commit that referenced this pull request May 10, 2015
implement FromStr for Url
@SimonSapin SimonSapin merged commit db92d21 into servo:master May 10, 2015
2 checks passed
2 checks passed
continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@SimonSapin
Copy link
Member

SimonSapin commented May 10, 2015

Published as v0.2.43 on crates.io.

@blaenk
Copy link
Contributor Author

blaenk commented May 10, 2015

Sweet thanks!

On Sunday, May 10, 2015, Simon Sapin notifications@github.com wrote:

Published as v0.2.43 on crates.io.


Reply to this email directly or view it on GitHub
#107 (comment).

  • Jorge Israel Peña
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.