Default value type wrapper

[sled]

• Default value type wrapper
• Collections should be empty and not nil by default
• Specs

[lwe]

Looks okay so far, but I'd not merge collection stuff into the same PR.

[sled]

oh wow this also directly does empty collections! However there seems to be an issue with one spec. Root cause is that the model's initializer is called with a string, which can't be coerced into hash with .to_h (done here: eb3b764#diff-889830c19bf22e0fcc7462d3a31efc5aR42)

Failures:

  1) Porro::Types::Object compliance behaves like a Type #load does not raise an exception
     Failure/Error: expect { subject.load(data) }.to_not raise_error

       expected no Exception, got #<NoMethodError: undefined method `to_h' for "data":String> with backtrace:
         # ./lib/porro/model.rb:42:in `initialize'
         # ./lib/porro/types/object.rb:24:in `new'
         # ./lib/porro/types/object.rb:24:in `load'
         # ./spec/support/shared_type.rb:10:in `block (4 levels) in <top (required)>'
         # ./spec/support/shared_type.rb:10:in `block (3 levels) in <top (required)>'
     Shared Example Group: "a Type" called from ./spec/porro/types/object_spec.rb:14
     # ./spec/support/shared_type.rb:10:in `block (3 levels) in <top (required)>'

Further investigation:

https://github.com/at-point/porro/blob/sled-empty-collections/spec/porro/types/object_spec.rb#L14

RSpec.describe Porro::Types::Object do
  subject { described_class.new(Address) }

  context 'compliance' do
    before { allow(subject).to receive(:warn).with(/unable to dump object of type Address/) }
    it_behaves_like 'a Type'
  end
  ...
end

Is tested against https://github.com/at-point/porro/blob/sled-empty-collections/spec/support/shared_type.rb#L10

RSpec.shared_examples 'a Type' do |supports: %w{blankify strip}|
  let(:data) { 'data' }

  context '#load' do
    it 'does not raise an exception' do
      expect { subject.load(data) }.to_not raise_error
    end
  end

  ...
end

Which results in a call like:

Address.new("data") which fails, since Address is a Porro model and expects something .to_hashable as constructor argument.

[lwe]

Any chance we can avoid that and add a guard in the initializer / to_h, also when e.g. nil is passed?

[lwe]

something like

params = params.respond_to?(:to_h) ? params.to_h : {}
attributes.merge(params)

[lwe]

and it need specs, also that a load is done on initialize

[sled]

nil is not a problem for vanilla ruby nil.to_h => {}, however isn't it an user error if something non-hashy is passed to the model constructor?

[lwe]

agree that it is a user error - we could add a workaround maybe in the shared specs?

[lwe]

glaube da würd ich eher eifach default nöd implementiere, so gsehts eso us als obs implemntiert wär, für das hämmer ja de supports: [] array

[sled]

@lwe can you check the specs? not sure about the foreach loop there ;)

What I want to test is:

Given a subject of any type with a default value set to:

• nil
• a non-nil literal
• a callable returning a literal

It should return the default literal if loaded with nil and dump the default literal if dumped with nil.

[sled]

uhh I cheated, will fix ;)

[sled]

There's a gotcha with the default value, in combination with an enum.

attribute :month, Porro::Types::Enum.new(1..12).default(15)

This defaults to 15 although 15 is not "valid" with respect to the enum, however the cycle "Porro -> Dump -> Porro" works fine, keeping the 15.

I think this is more a "definition" question how we want to handle this... Keeping it as is or a possible alternative could be to ask the underlying type whether it accepts? the default value - if it doesn't we could raise an ArgumentError stating that your document definition is illogical.

[sled]

this line is a bit controversial...

Doing a short-ciruit here like previously @wrapper.load(value) || default_proc.call allows invalid values to be assigned i.e

attribute :gender, Porro::Types::Enum.new(%w{male female}).default('hybrid')

will lead to Model.new.gender to return 'hybrid' although it's not a valid value for the enum.

The variant @wrapper.load(value || default_proc.call) will fail some tests, i.e

  attribute :gender, Porro::Types::Enum.new(%w{male female}).default('female')

  it 'converts anything else to default (female)' do
    expect(subject.send(method, 'FOOBAR')).to eq 'female'
  end

Will fail because value == 'FOOBAR' and default_proc.call == 'female' so the call looks like @wrapper.load('FOOBAR' || 'female') which will favor the 'FOOBAR' and the load returns nil.

[sled]

whoops... make sure to check the "outdated diff" comments ;)

[lwe]

@sled what is stopping us from changing Enum to the following:

class Enum
  include Base

  def initialize(values, default = nil)
    # ... setup
    self.default(default)
  end

  def default(value)
    raise ArgumentError, 'invalid value' unless ...
    @default_proc = value.respond_to?(:call) ? value : -> { value }
    self
  end
end

i.e. why not simply override default(value) for Enum and not use the default implementation, because that way - you could even raise an ArgumentError or something, of course though this would be harder with Proc's, so the big Q is... does it make any sense, to enforce only allowed values as default? 😄

In the end that way I'm sure we can properly handle defaults in a general and Enum-specific way.

[sled]

Sure we can directly do it in the enum. The big Q is whether the load/dump logic should be applied to the default as with any other other value. E.g attribute :birthday, Porro.date.default(-> { Date.today }) should dump to something like "2012-04-23T18:25:43.511Z" and not a <#Date: ....> and a Porro.date.default('not a date') should dump to nil.