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

Do not ignore the default option for collection attributes #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Verseth
Copy link

@Verseth Verseth commented May 7, 2024

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 the default option is disabled it is very hard to achieve.

I modified the behaviour so that collection attributes replace the default proc with -> { [] } only when default is nil.

This preserves the original behaviour of collection attributes having an empty array by default when no default option is passed. But the provided non-nil default is respected.

@kgiszczak
Copy link
Owner

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.

@kgiszczak
Copy link
Owner

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.

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.

None yet

2 participants