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.

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

Merged
merged 1 commit into from Oct 27, 2017
Merged

HTTP::Client: use required named argument form instead of post_form and put_form #5139

merged 1 commit into from Oct 27, 2017

Conversation

asterite
Copy link
Member

Fixes #5136

@asterite asterite self-assigned this Oct 17, 2017
@straight-shoota
Copy link
Member

headers and tls should not be forced named arguments to keep the signature similar to the existing methods with form: being the only difference.
I would also add notes to the docs of the methods which accept IO and String that using form is exactly the same as body + Content-Type header.


{% 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".
Copy link
Member

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)`

Copy link
Member Author

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.

@asterite
Copy link
Member Author

@straight-shoota No. If headers is not a required named argument it becomes very confusing in my opinion. I don't want to change it.

@asterite
Copy link
Member Author

In fact I would probably go the other way and make headers be a required named arguments for the non-form versions. Since an http request can specify body and headers but both of these are optional, relying on argument order is bad and bug-prone.

@asterite
Copy link
Member Author

Actually, I'll try to change it if possible, there's probably no confusion because the types are different.

@straight-shoota
Copy link
Member

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 headers and tls in one variant of the same method can be optional positional arguments, in the other variant they need to be named arguments.

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.

@RX14
Copy link
Contributor

RX14 commented Oct 26, 2017

I agree that we should make headers etc. be required named arguments in a later pr.

@straight-shoota
Copy link
Member

Why later?

@asterite
Copy link
Member Author

I'm actually not sure that headers must be required. It was a rushed reply of mine 😊

@RX14
Copy link
Contributor

RX14 commented Oct 26, 2017

It shouldn't be required to be present, it should be required to be a named argument, not a positional one.

@asterite asterite added this to the Next milestone Oct 27, 2017
@asterite asterite merged commit 1ac8880 into crystal-lang:master Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants