-
-
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
Adding put_form to HTTP::Client. Fixes #4181 #4190
Adding put_form to HTTP::Client. Fixes #4181 #4190
Conversation
src/http/client.cr
Outdated
request.headers["Content-type"] = "application/x-www-form-urlencoded" | ||
exec request | ||
end | ||
{% for http_method in ["post", "put"] %} |
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.
You could also add patch
to the list. (%w(post put patch)
)
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.
Ok, I wasn't sure if that would be a wanted change or not so I left it off. I'll add it in though.
@jwoertink if you rebase , maybe @RX14 can take a look ? |
yikes! I think I did that wrong... I did a fetch and rebase. Looks like it pulled in all the commits since I originally did this back in march 😝 How I do I squish all those? |
@RX14 or some other maintainer can squash them in the git PR window , or you can do it (Goole is your best friend ) |
Woah, there went something terribly wrong. It seems you've rebased on a wrong commit and now all commits to master have been rebased on that. Why is the commit |
481ea67
to
ec049a0
Compare
ok, much better. Thanks @straight-shoota 😄 |
src/http/client.cr
Outdated
# ``` | ||
def {{http_method.id}}_form(path, form : String | IO, headers : HTTP::Headers? = nil) : HTTP::Client::Response | ||
request = new_request({{http_method.upcase}}, path, headers, form) | ||
request.headers["Content-type"] = "application/x-www-form-urlencoded" |
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.
While you're at it, could you please change all Content-type
to Content-Type
with capital T
? Apart from this file we have it this way everywhere and that's the spelling recommended by RFC 7231.
src/http/client.cr
Outdated
exec request | ||
end | ||
{% for http_method in %w(post put patch) %} | ||
# Executes a {{http_method.id.upcase}} with form data. The "Content-type" header is set |
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.
A) Maybe add "request" after the method type? => Executes a POST request with form data...
src/http/client.cr
Outdated
request.headers["Content-type"] = "application/x-www-form-urlencoded" | ||
exec(request) do |response| | ||
yield response | ||
# Executes a {{http_method.id.upcase}} with form data and yields the response to the block. |
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.
dito A)
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.
Thank you! Could you put this in, too?
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.
oops! Missed one. Sure thing
src/http/client.cr
Outdated
exec request | ||
end | ||
{% for http_method in %w(post put patch) %} | ||
# Executes a {{http_method.id.upcase}} with form data. The "Content-type" header is set |
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.
B) Maybe add "and returns a Response
." =>... with form data and returns a `Response`.
src/http/client.cr
Outdated
body = HTTP::Params.encode(form) | ||
post_form path, body, headers | ||
end | ||
# Executes a {{http_method.id.upcase}} with form data. The "Content-type" header is set |
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.
dito A) B)
src/http/client.cr
Outdated
body = HTTP::Params.encode(form) | ||
post_form(path, body, headers) do |response| | ||
yield response | ||
# Executes a {{http_method.id.upcase}} with form data and yields the response to the block. |
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.
ditto A)
src/http/client.cr
Outdated
# end | ||
# ``` | ||
def {{http_method.id}}_form(path, form : Hash(String, _) | NamedTuple, headers : HTTP::Headers? = nil) | ||
body = HTTP::Params.from_hash(form) |
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.
dito C)
src/http/client.cr
Outdated
def self.post_form(url, form : String | IO | Hash, headers : HTTP::Headers? = nil, tls = nil) : HTTP::Client::Response | ||
exec(url, tls) do |client, path| | ||
client.post_form(path, form, headers) | ||
# Executes a {{http_method.id.upcase}} with form data. The "Content-type" header is set |
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.
dito A) B)
src/http/client.cr
Outdated
exec(url, tls) do |client, path| | ||
client.post_form(path, form, headers) do |response| | ||
yield response | ||
# Executes a {{http_method.id.upcase}} with form data and yields the response to the block. |
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.
dito A)
src/http/client.cr
Outdated
# response = client.{{http_method.id}}_form "/", {"foo" => "bar"} | ||
# ``` | ||
def {{http_method.id}}_form(path, form : Hash(String, _) | NamedTuple, headers : HTTP::Headers? = nil) : HTTP::Client::Response | ||
body = HTTP::Params.from_hash(form) |
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.
C) HTTP::Params.from_hash
has been renamed to HTTP::Params.encode
(#4205). That's why the specs are failing.
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.
Great!
So... Who do I have to buy a drink for to get this merged? 😂 |
yay! Thanks guys 😂 |
This commit adds in a helper
put_form
method forHTTP::Client
. The only real change is that it submits a "PUT" request instead of "POST". Since the code seems to be almost identical to thepost_form
methods, I put it in a macro which would allow for apatch_form
later if that's wanted.