trailblazer / representable

Maps representation documents from and to Ruby objects. Includes JSON, XML and YAML support, plain properties and compositions.
http://trailblazer.to/2.1/docs/representable.html
MIT License
689 stars 108 forks source link

Allow defining `:default` as lambda #263

Open yogeshjain999 opened 2 years ago

yogeshjain999 commented 2 years ago

Allow :default option on property to be accepted as static value or any callable.

property :album, default: "Spring"
property :album, default: ->(represented:, **) { represented.default_album }
esambo commented 2 years ago

I just tried using this in a rails app and updated the Gemfile:

gem "representable", github: "yogeshjain999/representable", branch: "default-lambda"

But I struggle to use it: bin/rails console

Message = Struct.new(:timestamp, :items, keyword_init: true)
Item = Struct.new(:id, :name, keyword_init: true)
class MessageRepresenter < Representable::Decorator
  include Representable::JSON
  nested :metadata, default: {} do
    property :timestamp,
      # default: ->(represented:, **) { represented.timestamp || Time.now.iso8601(3) } # msg.timestamp.call # => ArgumentError (missing keyword: represented)
      # default: ->(options) { Time.now.iso8601(3) } # msg.timestamp.call # => ArgumentError (wrong number of arguments (given 0, expected 1))
      default: -> { Time.now.iso8601(3) } # msg.timestamp.call # works
  end
end

msg = Message.new
MessageRepresenter.new(msg).from_json('{}')
msg.timestamp # => #<Proc:0x00007fd748559818@(irb):9 (lambda)>

It returns a lambda, which is the old behavior on master. I even tried cloning your repo locally, incremented the version and build a new local gem and then pointed my Gemfile path to it, but that didn't help either.

BTW, it would be really nice to also add some tests for collection and not just property. Collection seem to behave differently if the collection key is present in the input (with an empty array value), or if the key is not present at all.

yogeshjain999 commented 2 years ago

Good point @esambo , added tests for collections and nested attributes!

Although, I don't have to explicitly call the procs as per your example 🤔

Can you confirm from your Gemfile.lock that it's using my branch ?

yogeshjain999 commented 2 years ago

Sorry for the silence in between @esambo, I had some other stuff to look into. I'll reply to above comment soon.

esambo commented 2 years ago

No problem, life can sometimes be very busy. It seems that these tests are quite complicated and not very descriptive. My impression is that it would be helpful to split up the test setup for different scenarios, and to then explicitly name the tests setup and expected behavior being tested. I would find it very helpful to see all the supported behaviors and their edge cases being tested, which would also help to prevent accidental regression.