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

Prefer TryInto over TryFrom in builders #727

Closed
SmolPatches opened this issue Nov 23, 2024 · 2 comments · Fixed by #730
Closed

Prefer TryInto over TryFrom in builders #727

SmolPatches opened this issue Nov 23, 2024 · 2 comments · Fixed by #730
Labels
A-request Area: Request A-response Area: Response E-easy Effort: easy. Start here :D S-feature Severity: feature. This adds something new.

Comments

@SmolPatches
Copy link
Contributor

I have been looking at the API for this crate as someone relatively new to Rust.
Amongst the code, I noticed this method deceleration for the requests::Builder struct

pub fn method<T>(self, method: T) -> Builder
where
    Method: TryFrom<T>,
    <Method as TryFrom<T>>::Error: Into<Error>,

Initially I was confused as to the reasoning that a constraint on Method was defined here.
Since Method seems to be unused in the function signature.
After looking deeper I realize the point of this constraint is to limit T to values that can be converted to Method::try_from.
Is there any reason that the where clause doesn't use the reciprocal trait TryInto instead?
For example,

pub fn method<T>(self, method: T) -> Builder
where
    T TryInto<Method>,
    <T as TryInto<Method>>::Error: Into<Error>,

Idk about others but this would have been clearer to me.
I was curious if there was a reason as to why the original was chosen over this second solution, which I think is more clear(imo).

@seanmonstar
Copy link
Member

I can't honestly remember why this style was chosen. I thought it was because of the way From and Into played together, but it works the opposite to what would be supported with this syntax: all impl From automatically implement Into, but not the reverse. That's why libstd recommends:

Prefer using Into over using From when specifying trait bounds on a generic function. This way, types that directly implement Into can be used as arguments as well.

So, it would indeed be better to switch them all to TryInto.

Would you like to submit a PR doing so?

@seanmonstar seanmonstar changed the title Discussion: request::Builder where clause clarity Prefer TryInto over TryFrom in builders Nov 25, 2024
@seanmonstar seanmonstar added E-easy Effort: easy. Start here :D S-feature Severity: feature. This adds something new. A-request Area: Request A-response Area: Response labels Nov 25, 2024
@SmolPatches
Copy link
Contributor Author

SmolPatches commented Nov 25, 2024

Thanks for clarifying, and I have no problem making these changes in a PR :)

seanmonstar pushed a commit that referenced this issue Nov 28, 2024
This makes requests::Builder trait bound easily readable and consistent w/ stdlib recommendations. It also technically _increases_ the amount of types that could meet the bounds, because of how TryFrom/TryInto interact.

Closes #727 

Co-authored-by: rob <mdnlss@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-request Area: Request A-response Area: Response E-easy Effort: easy. Start here :D S-feature Severity: feature. This adds something new.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants