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

Evaluate expose block in instance context #28

Closed
MichaelXavier opened this issue Oct 14, 2013 · 3 comments · Fixed by #30
Closed

Evaluate expose block in instance context #28

MichaelXavier opened this issue Oct 14, 2013 · 3 comments · Fixed by #30

Comments

@MichaelXavier
Copy link
Contributor

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:

class ProductEntity < Grape::Entity
   expose :local_prices do
      prices.select(&:local?)
   end
   expose :web_prices do
      prices.select(&:web?)
   end
private

  def prices
    @prices ||= expensive_api_call
  end
end

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:

class ProductEntity < Grape::Entity
   expose :local_prices
   expose :web_prices
   expose :attribute_on_the_product
private

  def web_prices
    prices.select(&:web?)
  end

  def local_prices
    prices.select(&:local?)
  end

  def prices
    @prices ||= expensive_api_call
  end
end

Thoughts?

@mbleigh
Copy link
Contributor

mbleigh commented Oct 14, 2013

This is already possible, just without the instance_eval context. Rather, the block is provided the instance value:

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

@mbleigh mbleigh closed this as completed Oct 14, 2013
@MichaelXavier
Copy link
Contributor Author

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:

entity_test.rb:16:in `block in <class:ProductEntity>': undefined method `prices' for "you'd better not call prices on me":String (NoMethodError)
    from /home/eluns/.rvm/gems/ruby-2.0.0-p247/gems/grape-entity-0.3.0/lib/grape_entity/entity.rb:369:in `call'
    from /home/eluns/.rvm/gems/ruby-2.0.0-p247/gems/grape-entity-0.3.0/lib/grape_entity/entity.rb:369:in `value_for'
    from /home/eluns/.rvm/gems/ruby-2.0.0-p247/gems/grape-entity-0.3.0/lib/grape_entity/entity.rb:333:in `block in serializable_hash'
    from /home/eluns/.rvm/gems/ruby-2.0.0-p247/gems/grape-entity-0.3.0/lib/grape_entity/entity.rb:331:in `each'
    from /home/eluns/.rvm/gems/ruby-2.0.0-p247/gems/grape-entity-0.3.0/lib/grape_entity/entity.rb:331:in `inject'
    from /home/eluns/.rvm/gems/ruby-2.0.0-p247/gems/grape-entity-0.3.0/lib/grape_entity/entity.rb:331:in `serializable_hash'
    from entity_test.rb:29:in `<main>'

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@ec653d2

@dblock
Copy link
Member

dblock commented Oct 15, 2013

@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.

MichaelXavier added a commit to MichaelXavier/grape-entity that referenced this issue Oct 15, 2013
MichaelXavier added a commit to MichaelXavier/grape-entity that referenced this issue Oct 15, 2013
MichaelXavier added a commit to MichaelXavier/grape-entity that referenced this issue Oct 15, 2013
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 a pull request may close this issue.

3 participants