-
Notifications
You must be signed in to change notification settings - Fork 2
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
Adds mechanism for converting Fleece schemas to Aeson Values #49
Conversation
This allows for the use of Fleece schemas in defining `ToJSON` instances, like we do using `toParser` for `FromJSON` instances. Originally the plan was to replace the existing `Encoder` using `Encoding` and `Series` with one that uses `Value` and adding the `toEncoding` step in `encode` and `encodeStrict`. This results in a different JSON encoding, however - insertion order for key/value pairs is not preserved when using `Value`. This resulted in test failures because while the same fields are present, they may appear in a different order, so the encoded bytestrings won't always match.
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 think the overall desire for this is compelling given the initial message. In particular this certainly seems to help a gradual transition to using json-fleece
.
Just had some small changes/questions.
-- the same and will be decoded the same as `toJSON (toValue a)`, it will not | ||
-- be exactly the same bytestring produced by the two. | ||
-- | ||
data ToValue a |
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 should be a newtype
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.
Should I make the same change for other similar types? (Encoder
, Decoder
, etc)
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 think we likely should, but I don't want to make a bunch of extra work for this PR.
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 don't actually think we can make this a newtype
since it's taking two arguments - the name and the Value
-producing function.
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.
Oh, yes clearly
{-# LANGUAGE TypeOperators #-} | ||
|
||
module Fleece.Aeson.ToValue | ||
( ToValue (..) |
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.
Do we need to export the construction here at all, or is it always going to be built by the FC.Fleece
instance? If it's the later, I'd say we just export the type. For the former, export a constructor function instead.
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 don't think we have to, but I did it here to be consistent with other modules (Encoder
, Decoder
, etc) where we export the constructor. Should we only export the type in those cases, too?
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 would think so, but again, I think that change could easily be done after this.
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.
Done in 1021958
.
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.
One reason many of these constructors are exported is to ensure that end users who want to add new primitives to the schema language via their own classes can create instances of their type classes for the core Fleece instance types
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.
We're currently using the data constructor for Encoder in Schmods: Service.Wingspan
uses it to make an encoder for Beeline that emits invalid JSON and should never actually get invoked.
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.
Also using the data constructor in Data.Accessorial to use Aeson's type class instances ToJSON inside an Fleece Encoder
This allows for the use of Fleece schemas in defining
ToJSON
instances, like we do usingtoParser
forFromJSON
instances.Originally the plan was to replace the existing
Encoder
usingEncoding
andSeries
with one that usesValue
and adding thetoEncoding
step inencode
andencodeStrict
. This results in a different JSON encoding, however - insertion order for key/value pairs is not preserved when usingValue
. This resulted in test failures because while the same fields are present, they may appear in a different order, so the encoded bytestrings won't always match.