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.

Sign up for GitHub

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

Jump to bottom

Remove parsing from Request::param #709

Merged
merged 3 commits into from
Oct 3, 2020
Merged

Remove parsing from Request::param #709

merged 3 commits into from
Oct 3, 2020

Conversation

yoshuawuyts
Copy link
Member

This PR removes the parse portion of Request::param, making that an explicit step that needs to be called by end-users. This was inspired by my analysis in rust-lang/rust#75435 (comment) on similar ergonomics issues for a stdlib API. As a result many of our tests that want to extract a &str or String from the params are more intuitive.

jbr
jbr reviewed Oct 2, 2020
View reviewed changes
Copy link
Member

@jbr jbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This &str behavior makes sense, but might we also want a parse_param() with the current behavior?

@yoshuawuyts
Copy link
Member Author

@jbr I think it should be easy enough to call .parse() on the output of .param() to convert it into another type. I don't love the way we require a custom error enum just for that right now, and would prefer if we could remove that entirely in favor of letting people handle the errors individually.

@jbr jbr self-requested a review October 3, 2020 21:17
jbr
jbr approved these changes Oct 3, 2020
View reviewed changes
Copy link
Member

@jbr jbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On further review, 👍 Would merge if it were fast-forward, but looks like it needs a rebase

@jbr jbr merged commit 0a1327a into main Oct 3, 2020
@delete-merged-branch delete-merged-branch bot deleted the param-error branch October 3, 2020 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbr jbr jbr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yoshuawuyts @jbr