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

add TryFrom impls for path, scheme, uri and authority #308

Closed
wants to merge 1 commit into from

Conversation

prasannavl
Copy link
Contributor

No description provided.

@seanmonstar
Copy link
Member

Are the traits actually stable yet?

@prasannavl
Copy link
Contributor Author

prasannavl commented Apr 2, 2019

Ok. This is actually rather confusing. I got a clippy notification that they were stable and had removed all the feature attributes from my projects over the last 2 weeks.

But now, after your comment is the first time, I'm actually looking through the rust changelog -- couldn't find it, and rust-lang/rust#58015 rust-lang/rust#33417 seems to indicate it may not have. Hmm.

@prasannavl
Copy link
Contributor Author

Ok. Just tracked it down: rust-lang/rust#58302
Looks like it has been, a few weeks ago. Not sure if it has landed on stable yet.

@carllerche
Copy link
Collaborator

Thanks for the PR. We will have to wait until TryFrom lands on stable.

Also, currently, the http crate targets a minimum Rust version of 1.20. Adding support for std::TryFrom will require a bit of thought. I am not sure what the best strategy will be.

@prasannavl
Copy link
Contributor Author

Sounds good. I probably had mistaken the clippy nightly build for a stable one.

@carllerche also, a quick stab at the process:

  • Move the actual impl to a private fn in the module.
  • current functions call the above private impl.
  • Feature gating the TryFrom impl on rustc version (an added conditional flag with build.rs being as the current pseudo standard way to do it I believe?) that calls into the above private impl.

@carllerche
Copy link
Collaborator

Unfortunately, the HttpTryFrom trait is not identical to the one in std. So, removing HttpTryFrom will have to be punted until 0.2. However, we probably can add a blanket impl of HttpTryFrom for T: std::TryFrom.

@prasannavl
Copy link
Contributor Author

Ah. I'd have thought to simply leave HttpTryFrom as it is, and just extracting it's contents into a common function that's used by both, and dealing with the edge cases (if any) where-ever needed. Since the blanket is only useful on rustc 1.3x+ where TryFrom has landed.

@seanmonstar
Copy link
Member

There's now a 0.2.x branch to start making these changes, and #317 to track the complete replacement of HttpTryFrom with TryFrom.

Would you like to continue this PR?

@prasannavl
Copy link
Contributor Author

prasannavl commented May 30, 2019

Sure. This is definitely something I'd like to see.

Is this planned to be backward compatible with older versions of rustc still? Or is it post 1.3x where this was stabilized?

@seanmonstar
Copy link
Member

Is this planned to be backward compatible with older versions of rustc still?

I had just assumed that we couldn't really support both, as there'd be conflicts, but maybe it works? I haven't really thought about that...

@prasannavl
Copy link
Contributor Author

Since it's not a breaking change in rustc and just an addition, I think we can - but will just have to done with config flags, and build.rs gates - Just moving ahead is probably much easier with a nicer codebase.

Personally, I don't really see much benefit to jumping through hoops to support older rustc.

@seanmonstar
Copy link
Member

Yea, we're already needing a new rustc version for other breaking changes, so cleaner seems best.

I suppose if there's a not-too-hard way that we can support it via deprecation, that'd be nice for users upgrading, but deprecation and changing trait bounds is hard to do in Rust.

@seanmonstar seanmonstar added this to the 0.2 milestone May 30, 2019
@davidbarsky
Copy link
Contributor

@prasannavl I'm just catching up on this issue, but this is something I wanted for a bit in http. I'd be happy to pick this PR up if you no longer have the bandwidth to drive this?

@prasannavl
Copy link
Contributor Author

So sorry, this has been held up so long. Couldn't make time.

@davidbarsky, I had done some partial bits - was planning on finishing this the coming weekend. However, if you'd like to take this up before, that would be great! :)

@davidbarsky
Copy link
Contributor

@prasannavl No worries! I don't want to trample over your work, hence me asking!

@prasannavl
Copy link
Contributor Author

@davidbarsky - I'm afraid I couldn't put much time into it. So created a new PR on the 0.2.x branch with a small change set containing just the bits here so it's unblocked for further work.

Think we can close this?

@weifenbachja - would you mind reviewing this at #333? I don't think I have the necessary rights to add reviewers there.

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.

5 participants