-
Notifications
You must be signed in to change notification settings - Fork 321
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
Type-directed optional fields #1023
Conversation
tests/UnitTests/OptionalFields.hs
Outdated
-- c.f. https://github.com/haskell/aeson/pull/839#issuecomment-782453060 | ||
data POC = POC | ||
{ x :: Nullable Int -- Field is required, but can be null. | ||
, y :: Undefineable Int -- Field is optional, but cannot be null. | ||
, z :: NullOrUndefineable Int -- Field is optional, and can be null. | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests verify that the PR supports your desired behavior, @bergmark :)
(c.f. #839 (comment))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Having omitField :: a -> Bool
is indeed simpler that what I had in #839.
There probably should be something in ToJSON1
and FromJSON1
classes, as e.g. instance for Fix
relies on them. How Fix Maybe
should be serialised?
I was able to get the generic classes down to one method like you suggested. I still need to work on the |
I marked this as draft. Please ping when you seem happy again. |
92d0723
to
59c7a73
Compare
@phadej I'm pretty happy with it, now. I'm interested in your feedback :-) |
Oh, also, @phadej, I do have some ideas about changes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
I have further things to do for this, but I think it's easier and fasterfor me to them myself rather than try to explain them.
Could you squash your changes into own commit (and rebase on recent master
), so I can continue from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some doc updates necessary, but this is super cool. ~268 lines of tests for new functionality means that this is really a +145 / -177 LOC change that adds a feature and removes some warts! Well done.
-------------------------------------------------------------------------------- | ||
-- IncoherentInstancesNeeded | ||
-------------------------------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄 Briliant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was like, "whelp, we don't need this anymore"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your help and ideas, btw, @parsonsmatt. The TH was trivially-easy, but the Generics threw me for a loop.
Done. Thanks, @phadej ! |
If you think so please mark or even suggest amendments. I'm mostly happy with docs in this PR. |
@phadej there were two places my docs had minor mistakes, but those were taken care of :-) |
* Adds 'omitField' to 'ToJSON' * Adds 'omittedField' to 'FromJSON'
#1039 is merged. Thanks for making the initial version! |
This PR makes a backwards-compatible change to
ToJSON
andFromJSON
that allows the instance to configure the optionality of record fields of that type when using Generic deriving or Template Haskell deriving.Specifically, we make the following use-facing changes:
and
The new methods have default implementations that preserve the current behavior of Generically-derived and Template Haskell created instances.
Instance authors can use these methods to configure how to represent fields of this type as omitted from a JSON record.
As an added benefit, this PR eliminates the needs for special-case handling of
Maybe
andOption
, significantly simplifying the Generic and Template Haskell code.This PR solves #646 and #792, and it's similar to #839 by @phadej.
Please let me know what you think, if this PR is a suitable addition to Aeson, and if there's anything about it you'd like me to fix or change.
Thanks much!