whichdigital / active-rest-client

ActiveRestClient API Client
https://rubygems.org/gems/active_rest_client
MIT License
386 stars 44 forks source link

Make params accessible #83

Closed nathanhoel closed 9 years ago

nathanhoel commented 9 years ago

For instance makes before_request and after_request methods able to access the params list like this

class Resource < ActiveRestClient::Base
  before_request :set_custom_headers

  def set_custom_headers(name, request)
    id = request.params[:id]
    request.headers["x-vy-organization"] = id
  end
end

Without access to params we had to use a regular expression or string manipulation on the request.url to find the id in the url.

coveralls commented 9 years ago

Coverage Status

Coverage remained the same at 99.17% when pulling 85bce52ceaca23a029b68604500c914a0b864892 on nathanhoel:open-params into ea0a7af1ec848e21a92e126f53a2071e1427a5b0 on whichdigital:master.

andyjeffries commented 9 years ago

The only problem I have with this PR, is that accessing params on request may make users think they can change the params and have it take effect. If they want to change them, they have to use get_params and post_params, which are both already available on the request object because prepare_params has already been called, splitting params in to the two objects before it gets to send(:_filter_request, ...).

So I'm concerned that we're giving people another way to access the params (which may cause confusion with the existing methods) that is less useful than the existing ways. There are ways around this (have params return an object that wraps both attributes, and when setting back on the params object update the main object) but I don't think it's very clear or particularly necessary.

I loved the idea though...

nathanhoel commented 9 years ago

I see what you are saying for sure.

When the params are being parsed into get_params and post_params the one problem is not all of the params make it into those objects. Any parameter used to create the url is not put in either object. It is these very params that we need access to inside the before_request method.

When parsing the parameters we could create a third object with these parameters that were consumed in the url, what do you think of that?

andyjeffries commented 9 years ago

I think that makes sense mate.

andyjeffries commented 9 years ago

Let me know when/if you re-work your PR to be that way.

andyjeffries commented 9 years ago

@nathanhoel did you plan on reworking the PR? Or shall I close this?

nathanhoel commented 9 years ago

We found a work around and so it's probably just not important. I can close since I don't plan on doing anything with it.