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

Pass adapter options through calls to render. #63

Merged
merged 2 commits into from
Sep 9, 2016
Merged

Conversation

drn
Copy link
Member

@drn drn commented Aug 30, 2016

This allows for functionality like this:

class UsersApi
  resource :users do
    get '/:id' do
      if conditional
        user
      else
        render user, serializer: ErrorSerializer, adapter: :attributes
      end
    end
  end
end

the only way to pass adapter options was via route_options, which doesn't work if we need to programmatically switch on any of the options (esp because they are cached in between requests as noted here ruby-grape/grape#918)

@drn drn force-pushed the adapter-options branch from 39958f7 to 41595fc Compare August 30, 2016 22:22
@drn drn force-pushed the adapter-options branch 2 times, most recently from c26577b to 2a98c3c Compare August 31, 2016 01:21
@drn drn assigned siong1987 and jrhe Sep 1, 2016
@dblock
Copy link
Member

dblock commented Sep 6, 2016

I like it. It needs a README update please and a rebase.

@drn
Copy link
Member Author

drn commented Sep 7, 2016

Great! I've updated the README. Let me know if you have any other suggestions

@dblock
Copy link
Member

dblock commented Sep 7, 2016

Thinking about this more, help me understand what's going on. Now all options get merged into render. So why do we have ams_meta vs. ams_adapter_options at all?

@drn
Copy link
Member Author

drn commented Sep 8, 2016

Hey @dblock, sorry for the late reply. The intention there was to not have the meta options pollute the main option pool. Ideally the ams_adapter_options would only be the subset of options explicitly used by AMS adapters, however I took a shortcut here and merged them all in.

I'll check into the internals of AMS and then look for a list of options and whitelist the options that can be passed along in ams_adapter_options. Would that solve your concerns?

@drn
Copy link
Member Author

drn commented Sep 8, 2016

Okay @dblock - I've pushed up another commit that both explicitly whitelists the adapter options that are passed along as well as adds support for optionally defined extra options. Let me know what you think

def render(resources, meta = {})
env['ams_meta'] = meta
def render(resources, extra_options = {})
extra_options.symbolize_keys!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're modifying the value passed in. You shouldn't. At least dup them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dblock
Copy link
Member

dblock commented Sep 9, 2016

This is a lot better. I have some minor-ish concerns above, will merge after those are fixed.

@drn
Copy link
Member Author

drn commented Sep 9, 2016

Okay! @dblock - I've addressed your comments. Thanks for the review!

@dblock dblock merged commit 2748d5b into master Sep 9, 2016
@dblock
Copy link
Member

dblock commented Sep 9, 2016

Merged.

@dblock
Copy link
Member

dblock commented Mar 23, 2017

Hey @drn interested in helping out with this gem? Maybe making a release?

@drn drn deleted the adapter-options branch April 21, 2017 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants