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

Option nullaryToObject to encode/decode nullary constructors as empty objects #926

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

epoberezkin
Copy link

@epoberezkin epoberezkin commented Jan 29, 2022

issue #925

  • Generic JSON/encoding/parse
  • TH JSON/encoding/parse
  • with rejectUnknownFields option, non-empty objects should fail (both Generic and TH
  • tests
  • make parsers dependent on the option, without relaxing the default behaviour

@epoberezkin
Copy link
Author

I've figured how to add this option to TH encoding, and added tests... For TH parsing, I suspect parseNullaryMatches should change but don't yet understand how...

cc @phadej

@epoberezkin
Copy link
Author

@phadej I think it's mergeable, and it would be great if we could upstream it - as I wrote, you can't use the library with Swift without this option.

Parsing for nullary constructors currently accepts both empty array and an object (object must be empty if rejectUnknownFields is set), so it will parse JSON generated irrespective of this option.

@epoberezkin epoberezkin changed the title Option nullaryToObject to encode/decode nullary constructors as empty objects (only Generic atm) Option nullaryToObject to encode/decode nullary constructors as empty objects Oct 6, 2023
Tagged . contextCons cname tname $ case v of
Array a | V.null a -> pure U1
| otherwise -> fail_ a
Object o | KM.null o || not (rejectUnknownFields opts) -> pure U1
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not right. You accept {} even if nullaryToObject is False, i.e. relaxing the parser. And vice versa, nullaryToObject = True case will accept [].

As I said, I'm not a fan of extending the kitchen-sink existing deriving mechanism. There are just too many interacting options.

I really suggest that you create your own generic serializing/deserializing fit for your needs. It will be

  • faster to compile
  • produce faster code

Copy link
Author

Choose a reason for hiding this comment

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

This is not right. You accept {} even if nullaryToObject is False, i.e. relaxing the parser.

That's right, it was intended - what's the harm here?
But happy to make it follow the option - I will do that, it's easy.

I really suggest that you create your own generic serializing/deserializing fit for your needs.

Well, our needs are very close to what aeson does, it's not our job to make a better JSON library.

Copy link
Author

Choose a reason for hiding this comment

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

@phadej so now these parsers depend on this option, so the default parsers are not relaxed. Tests are amended too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, our needs are very close to what aeson does,

aeson is not just Generic deriving. Generic deriving is just a (smalL) part (and IMO the not very great part) of a library, and is redone in other libraries, like https://hackage.haskell.org/package/deriving-aeson , https://hackage.haskell.org/package/autodocodec-0.2.1.0/docs/Autodocodec-Aeson.html, https://hackage.haskell.org/package/json-sop

Copy link
Author

Choose a reason for hiding this comment

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

that's ok, it amends TH too. We will be switching to TH anyway.

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.

2 participants