Open Verseth opened 1 month ago
Thank you, it looks good to me at first glance, but I'll have to review it more thoroughly. I don't remember right now, but this default might be required to not break some of the code paths.
Hey so I did a quick test and indeed it crashes on XML parsing.
Here's an example how to reproduce:
class User < Shale::Mapper
attribute :pets, Shale::Type::String, collection: true, default: -> { nil }
xml do
root 'user'
map_element 'pet', to: :pets
end
end
User.from_xml(<<~END)
<user>
<pet>foo</pet>
<pet>bar</pet>
<pet>baz</pet>
</user>
END
It was just a quick test, but I feel like there might be other places in the code that assumes the default will be an array. You can take a stab at fixing it, but I feel it will be a pretty big lift. If you do, please create extensive test coverage so we have confidence in it.
I noticed that the
default
attribute option is ignored and replaced with-> { [] }
for collection attributes.I find it annoying, especially when someone wants the collection to be
nil
by default. Because thedefault
option is disabled it is very hard to achieve.I modified the behaviour so that collection attributes replace the default proc with
-> { [] }
only whendefault
isnil
.This preserves the original behaviour of collection attributes having an empty array by default when no
default
option is passed. But the provided non-nildefault
is respected.