-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Adds URI#query_params method #8090
Adds URI#query_params method #8090
Conversation
spec/std/uri_spec.cr
Outdated
describe "#query_params" do | ||
it do | ||
uri = URI.parse("http://foo.com?id=30&limit=5#time=1305298413") | ||
uri.query_params.should eq(HTTP::Params.parse("id=30&limit=5")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, the spec would be more reliant if the HTTP::Params
instance was created from a spec. We're not testing Params.parse
here, but still...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? Something like:
expected_params = HTTP::Params.new({ "id" => ["30"], "limit" => ["5"]} )
uri.query_params.should eq(expected_params)
@rodrigopinto Could you please squash the commits except the first one? I'd like to merge this as two commits, a spec refactor and a feature addition. |
… of the query. Using @asterite suggestions as it is more idiomatic. Co-Authored-By: Ary Borenszweig <asterite@gmail.com>
0b88830
to
4bf0ca0
Compare
@straight-shoota it is done. |
Thank you @rodrigopinto |
This PR is for adding a
URI#query_params
which will generate aHTTP::Params
instance parsed from theURI#query
.Example:
👉 it closes #8085