Skip to content

Conversation

@FlorianRichardUPF
Copy link
Contributor

What does this PR do?

What are the observable changes?

Good PR checklist

  • Title makes sense
  • Is against the correct branch
  • Only addresses one issue
  • Properly assigned
  • Added/updated tests
  • Added/updated documentation
  • Properly labeled

Additional Notes

@FlorianRichardUPF
Copy link
Contributor Author

So this is the only way I found to make things work.

I put back Enumerable, otherwise it enforces us to us an array as input but we lose ActiveRecord relations (not sure of me, but couldn't serialize nested objects).

I've added a new case for Hashes to use CompositeObject.new(obj, opts).

@tgallice @AlexisMontagne What do you think about that ?

if obj.is_a? Array
ArraySerializer.new(obj, opts)
else
if obj.is_a?(Hash) || !obj.is_a?(Enumerable)

Choose a reason for hiding this comment

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

I think should wrap most of the cases, but if we have a Nil class passed here, it will jump inside of this if.
Should we consider this scenario?

Copy link
Contributor Author

@FlorianRichardUPF FlorianRichardUPF Jul 16, 2025

Choose a reason for hiding this comment

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

I think it is the same behaviour as before. There was an else that would catch everything else including Nil class.

Copy link
Member

@tgallice tgallice left a comment

Choose a reason for hiding this comment

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

:/

@FlorianRichardUPF FlorianRichardUPF merged commit f12322d into master Jul 16, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants