-
-
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
HTTP::Client: use required named argument form
instead of post_form and put_form
#5139
HTTP::Client: use required named argument form
instead of post_form and put_form
#5139
Conversation
|
|
||
{% for http_method in %w(post put patch) %} | ||
# Executes a {{http_method.id.upcase}} request with form data and returns a `Response`. The "Content-Type" header is set | ||
# Executes a {{method.id.upcase}} request with form data and returns a `Response`. The "Content-Type" header is set | ||
# to "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.
Suggestion to add to "application/x-www-form-urlencoded". Otherwise it is the same as using `#{{method.id}}(path, headers, body)`
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.
Maybe, but it's not relevant to this PR.
@straight-shoota No. If |
In fact I would probably go the other way and make |
Actually, I'll try to change it if possible, there's probably no confusion because the types are different. |
I'm not sure what would be confusing about having optional (named or positional) arguments together with a required named argument. But at least I'd find it confusing if So it should be either optional positional or optional named-only arguments everywhere. I'd stick to the existing signatures because there is little benefit from having them named-only. |
I agree that we should make headers etc. be required named arguments in a later pr. |
Why later? |
I'm actually not sure that |
It shouldn't be required to be present, it should be required to be a named argument, not a positional one. |
Fixes #5136