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

Allow defining :default as lambda #263

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yogeshjain999
Copy link
Member

@yogeshjain999 yogeshjain999 commented Jun 12, 2022

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
Copy link

esambo commented Jun 18, 2022

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
Copy link
Member Author

yogeshjain999 commented Jun 19, 2022

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 ?

Copy link

@esambo esambo left a comment

Choose a reason for hiding this comment

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

I was able to use this new lambda default in the cloned repo once I switched to the branch and executed:
irb -I lib -r representable:

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:, **_options) { Time.now.iso8601(3) }
  end
end

def parsed_timestamp(json)
  msg = Message.new
  MessageRepresenter.new(msg).from_json(json)
  msg.timestamp
end

# works as expected, and returns the current timestamp
parsed_timestamp '{}'
parsed_timestamp '{"metadata":{}}'

# does not work, and will not return the current timestamp
parsed_timestamp '{"metadata":{"timestamp":null}}'
parsed_timestamp '{"metadata":{"timestamp":""}}'
parsed_timestamp '{"metadata":{"timestamp":"NOW"}}'

But my example using a static default in a collection (that works on master) is broken and raises exceptions (in this feature branch):

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: :static_default,
      parse_filter: ->(fragment, _options) {
        runtime_default = Time.now.iso8601(3)
        fragment == :static_default ? runtime_default : fragment
      }
  end
  collection :items,
    class: Item,
    default: [{}], # ONLY works if collection key is missing from input. # this default value causes a warning!
    # [Declarative] Defaults#merge! and #call still accept arrays and automatically prepend those. This is now deprecated, you should replace `ary` with `Declarative::Variables::Append(ary)`.
    parse_filter: ->(fragment, options) {
      fragment.empty? ? [Item.new(name: "collection default")] : fragment
  } do
    property :id, default: 0
    nested :details, default: {} do
      property :name, default: "missing"
    end
  end
end

# Empty payload. Defaults work
json = '{}'
msg = Message.new
MessageRepresenter.new(msg).from_json(json)
Time.parse(msg.timestamp).year # => 2022 # GOOD, a dynamic runtime default value in a nested property
msg.items # => [#<struct Item id=0, name="missing">] # GOOD, both have defaults

# Payload with empty collection. Defaults are broken
json = '{"items":[]}'
msg = Message.new
MessageRepresenter.new(msg).from_json(json)
msg.items # => [#<struct Item id=nil, name="collection default">]
msg.items.first.id # => nil # BAD, no default


representer! do
property :id
property :title, default: "Huber Breeze" #->(options) { options[:default] }
property :album, default: ->(represented:, **) { represented.album || "Spring" }
collection :composers, instance: ->(*) { Composer.new } do
Copy link

Choose a reason for hiding this comment

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

It would be very helpful if this collection also showed how to use a default:.
BTW, is there a benefit of using instance: ->(*) { Composer.new } instead of class: Composer? The tests seem to pass either way.

property :name, default: "Unknown"
property :keywords, default: ->(represented:, **) { [represented.name.downcase] }
end
nested :metadata do
Copy link

Choose a reason for hiding this comment

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

It would be helpful if this nested also showed how to use a default:.

@yogeshjain999
Copy link
Member Author

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

@esambo
Copy link

esambo commented Jun 29, 2022

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.

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.

2 participants