-
Notifications
You must be signed in to change notification settings - Fork 961
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
Allow empty array to be used as a params in get requests. #477
Conversation
new_uri.query = query_string(new_uri) | ||
possible_query_string = query_string(new_uri) | ||
|
||
if possible_query_string != nil && possible_query_string != "" |
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.
Out of curiousity, what specifically made it so you need this?
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.
Well… I was doing baby steps and the change was required, but I double-checked and it is not required anymore :)
7e1c78f
to
92be470
Compare
@@ -201,7 +201,9 @@ def query_string(uri) | |||
query_string_parts << options[:query] unless options[:query].nil? | |||
end | |||
|
|||
query_string_parts.reject!(&:empty?) unless query_string_parts == [""] | |||
query_string_parts.reject! { |part| |
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.
I checked locally and this isn't needed to make any specs pass. Do we need a new spec or is this extra as well?
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.
Weird… I remember I needed this :/
Let me check :)
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.
Hey @jnunemaker thank you for the great review, it was necessary to change only HTTParty::HashConversions
eventually :)
92be470
to
db06d36
Compare
In this PR you mention that change of behavior should be only for get method. |
@DaftTrick I did all the tests based on GET, but I did not remember of seeing anything to mean that the behavior should be only to GET, so I think this code should work to POST too. Why? Any problem with that? P. S.: Sorry by the delay :) |
Fixes #469
Now we have:
And:
Besides this fix I'd like to know what happens if you have something like this:
HTTParty::Request.new(Net::HTTP::Get, 'http://google.com', query: { fake_array: [""] })