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

Support wrapping optional around primalize #6

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

christoomey
Copy link
Contributor

Prior to this change, nesting optional(primalize(FooSerializer)) would properly handle nil values, but would directly coerce the passed object as JSON rather than calling the specified serializer.

This change updates to call the nested primalize serializer with the object.


I'll be honest that I don't deeply understand all the functionality at play here so this solution is largely pattern matching on some of the other implementations, notably the Array coercion logic, but thankfully the test suite had solid coverage and helped point me in the correct direction, so functionally I think this is good, but happy to adjust implementation however might make sense.

Prior to this change, nesting `optional(primalize(FooSerializer))` would
properly handle `nil` values, but would directly coerce the passed
object as JSON rather than calling the specified serializer.

This change updates to call the nested primalize serializer with the
object.
Copy link
Owner

@jgaskins jgaskins left a comment

Choose a reason for hiding this comment

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

Thanks! I remember having an issue composing modifiers with primalize (and I think optional was the one I was using), but I ended up working around it and never revisited it.

I'll be honest that I don't deeply understand all the functionality at play here so this solution is largely pattern matching on some of the other implementations, notably the Array coercion logic, but thankfully the test suite had solid coverage and helped point me in the correct direction, so functionally I think this is good, but happy to adjust implementation however might make sense.

This looks right to me, too. The modifiers can be a little tricky, which is probably why I overlooked this.

@jgaskins jgaskins merged commit c9ef5af into jgaskins:master Sep 14, 2022
@jgaskins
Copy link
Owner

Also released v0.3.10 with this change

@christoomey
Copy link
Contributor Author

Thanks for the quick pickup, and again, thanks for the great library!

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