Skip to content

Add option :merge to expose. Allow to merge fields into nested hashes and into the root #204

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

Merged
merged 1 commit into from
Jan 29, 2016

Conversation

sagebomb
Copy link
Contributor

This also closes #138
Hi guys. Long time ago I dreamed about merging fields into nested hashes. And that's it. I found it useful and decided to make a PR.

We can do this:

expose :contact_info do
  expose :phone
  expose :address, merge: true, using: API::Entities::Address
end

this:

expose :something_useful, merge: true

And even this:

expose :profiles do
  expose :users, merge: true, using: API::Entities::User
  expose :admins, merge: true, using: API::Entities::Admin
end

It would be awesome if you take a look at this. Thanks!

@@ -1,7 +1,7 @@
0.5.1 (Next)
============

* Your contribution here.
Copy link
Member

Choose a reason for hiding this comment

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

Put this back please.

@dblock
Copy link
Member

dblock commented Jan 28, 2016

I like it, just amend the CHANGELOG please, and I'll keep this open for a bit, lets see what others say.

@uberjay
Copy link

uberjay commented Jan 28, 2016

Very timely for me! I was just poking around for solutions to this. Specifically, the ability to merge into the root hash. The one question I have, is what's the behavior on key collision, and can I control it. One idea is to support specifying a lambda to merge, and have it work like Hash#merge's block params:

expose :custom_attrs, merge: -> (k, v1, v2) { v1 }

I think this would do the trick -- in the OutputBuilder class:

if exposure.for_merge
  if exposure.for_merge.respond_to? :call
    @output_hash.merge! result, &exposure.for_merge
  else
    @output_hash.merge! result
  end
else
  @output_hash[exposure.key] = result
end

@sagebomb
Copy link
Contributor Author

Hi @uberjay .
Default behaviour is just to merge! it. So a new key replaces old one. I think to use lambda is a good idea.

@dblock
Copy link
Member

dblock commented Jan 29, 2016

I am going to merge this.

See what I did there? Merge ...

👍

dblock added a commit that referenced this pull request Jan 29, 2016
Add option :merge to expose. Allow to merge fields into nested hashes and into the root
@dblock dblock merged commit 2ba53e4 into ruby-grape:master Jan 29, 2016
@dblock
Copy link
Member

dblock commented Apr 6, 2016

@avyy Looks like this introduced a regression, can you please check out #213?

@sagebomb
Copy link
Contributor Author

sagebomb commented Apr 6, 2016

@dblock ok I'll check it a bit later 👍

@wleeper
Copy link

wleeper commented May 13, 2016

Any update on this? Just spent a lot of time chasing my tail due to this. I recommend pulling 0.5.1 or reverting this until it is resolved. This is a breaking change for more than just testing.

It breaks the wrapping of error messages in error! calls when using an entity model because the result returned to the present algorithm is no longer a Hash.

@sagebomb
Copy link
Contributor Author

@wleeper I working on this. Tomorrow I let you know if it's really better to revert that commit 🎱

@dblock
Copy link
Member

dblock commented May 14, 2016

I don't think yanking 0.5.1 now is a good option, there're lots of people using it in production and it doesn't break "everything". Looking forward to a fix.

@lucaskanashiro
Copy link

Any news?

@dblock
Copy link
Member

dblock commented Jun 26, 2016

@lucaskanashiro Looking forward to your PR!

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.

Feature request: Expose a Hash merged to the root, not as a sub key
5 participants