Skip to content
This repository was archived by the owner on Mar 8, 2019. It is now read-only.

Allow request arguments#4

Open
cschramm wants to merge 1 commit intoxing:masterfrom
cschramm:master
Open

Allow request arguments#4
cschramm wants to merge 1 commit intoxing:masterfrom
cschramm:master

Conversation

@cschramm
Copy link
Copy Markdown

@cschramm cschramm commented Oct 1, 2014

There are at least vendor resources where you need to set a body. Headers may also be useful.

@markschmidt
Copy link
Copy Markdown
Contributor

@cschramm I agree, that would be quite useful!

But I have to say, I don't like the fact to expose the oauth-gem specific arguments to the user very much. That will basically result in request(http_verb, url, options, body, other_options) which feels a bit rubbish to me.

What do you think about something like request(:post, "/foobar", {query_param: value}, {body: "", headers: ""}) or to have it more compact but with possible name conflicts request(:post, "/foobar", query_param: value, body: "", headers: "")?

@cschramm
Copy link
Copy Markdown
Author

cschramm commented Oct 1, 2014

I get your thoughts about the double options hash, but I simply would not call the parameters options and there's nothing wrong with params, body, headers (the headers aren't options to me either).

What improvement does your first suggestion provide? The second suggestion would be pretty nice to use and the possible conflicts are absolutely unlikely, but in any case it has a theoretical flaw...

@markschmidt
Copy link
Copy Markdown
Contributor

Well, my first suggestion would be at least a little bit more in line with what other libraries like typhoeus are doing. Also, having to different option hashes as the last method arguments - even though it's not pretty - is not that uncommon in rails-land (see for instance http://api.rubyonrails.org/classes/ActionView/Helpers/UrlHelper.html#method-i-link_to).

@cschramm
Copy link
Copy Markdown
Author

cschramm commented Oct 2, 2014

a) POST and PUT: http_verb, url, params, body, headers
Else: http_verb, url, params, headers
b) http_verb, url, params, body: body, headers: headers
c) http_verb, url, params: params, body: body, headers: headers

I probably got the wrong idea of why that call (a) feels rubbish to you. I thought you meant the two hashes, but you probably mean that a body argument is required to specify headers for POST and PUT requests, as it is the case in the oauth gem. That's of course improved in your first suggestion (b).

It may be a counter-argument that this interface is new to users who are already familiar with oauth's and that the implementation will get a little ugly since the semantics of the arguments depend on http_method. But especially this feels rubbish to me now, since I can now leave out the body, but not the params. It would be much more intuitive to have a single hash that accepts the keys :params, :body, and :headers (c), but that would break existing calls.

Anyway, here's an implementation for (b): cschramm@8344794

But for the mentioned reasons I like (a). What I'd like best is (c). We could check if options includes any of the three keys and fall back to the previous behavior otherwise. That would make it 'practically backward-compatible'.

@markschmidt
Copy link
Copy Markdown
Contributor

You're right, (c) sounds like the best solution and fallback to the previous implementation if none of params, body or headers is present, should be enough.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants