-
Notifications
You must be signed in to change notification settings - Fork 151
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
Evaluate expose block in instance context #28
Comments
This is already possible, just without the class ProductEntity < Grape::Entity
expose :local_prices do |product, options|
product.prices.select(&:local?)
end
expose :web_prices do |product, options|
product.prices.select(&:web?)
end
private
def prices
@prices ||= expensive_api_call
end
end |
I don't think that is correct. The code seems to pass in the object instance, not the entity instance. An example use case where you'd want to call methods on the entity instance instead of the object is if there are some fields that are particular to the API that you would not want on the model. Most of the time these methods on the entity will call into the model for something but then massage it for the entity. Here's a fully executable example require 'grape-entity'
require 'pp'
Price = Struct.new(:price, :local) do
def local?
local
end
def web?
!local?
end
end
class ProductEntity < Grape::Entity
expose :local_prices do |product, options|
product.prices.select(&:local?)
end
expose :web_prices do |product, options|
product.prices.select(&:web?)
end
private
def prices
@prices ||= [Price.new(3.50, true), Price.new(1.50, false)]
end
end
pp ProductEntity.new("you'd better not call prices on me").serializable_hash Output:
BTW I took a stab at the non-block approach on my fork and it came out quite clean, painless and didn't break any tests: |
@MichaelXavier I think you/re right. I'll take a PR (don't forget to update the CHANGELOG). @mbleigh lmk if you think I missed something. |
I noticed that the block version of an exposure pretty much runs without any usable context. The context it currently uses is the class of the entity, which doesn't really provide much, passing in the object being wrapped and the options.
I think it would be a lot cleaner and more intuitive if the block was evaluated in the context of the instance around here.
Here's an example use case where that cleans things up:
This doesn't work currently because the prices method is on the instance but the block gets evaluated on the class. Wouldn't want to add the prices method to the class because it is stateful.
Alternatively (and I prefer this), you could have exposures see if the entity responds to it first, and if not delegate to the object as normal. I think Roar does this and it is much easier to build a proper facade that way. Admittedly, it is a more breaking API change. For comparison:
Thoughts?
The text was updated successfully, but these errors were encountered: