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

Optional fields #1039

Merged
merged 2 commits into from
Jun 19, 2023
Merged

Optional fields #1039

merged 2 commits into from
Jun 19, 2023

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Jun 10, 2023

Continuation of #1023

Copy link
Contributor

@friedbrice friedbrice left a comment

Choose a reason for hiding this comment

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

I was thinking Aeson could retain backwards compatibility if instead of adding an argument to liftToJSON, a method liftToJSONWithOmit could be added. It could have the default implementation liftToJSONOmit _ = liftToJSON to preserve the behavior of people's existing instances. Similar additions to ToJSON2 and FromJSON1, FromJSON2 could be made.

@phadej
Copy link
Collaborator Author

phadej commented Jun 12, 2023

I was thinking Aeson could retain backwards compatibility

It's not worth it. There are barely any direct users of these https://hackage-search.serokell.io/?q=liftToJSON and arguably the ones who are (e.g. autocodec, insert-ordered-containers, should adapt and could adapt easily).

EDIT: In fact liftToJSONOmit _ = liftToJSON would make generic deriving "wrong" by default. The same problem as with toEncoding: people (e.g. yourself) forget to define it. The current design of toEncoding = toEncoding . toJSON makes doing wrong thing easy for the sake of backward compatibility which is moot now about a decade ago. This is an example while I'd just break things right away, instead of waiting indefinitely for people to start doing the right thing.

@friedbrice
Copy link
Contributor

@phadej That all makes a lot of sense. Thanks for explaining the situation to me. (Also, I'll start defining toEncoding!)

src/Data/Aeson/TH.hs Outdated Show resolved Hide resolved
@@ -130,10 +130,5 @@ templateHaskellTests =
thOneConstructorToJSONDefault `sameAs` thOneConstructorToEncodingDefault
, testProperty "OneConstructorTagged" $
thOneConstructorToJSONTagged `sameAs` thOneConstructorToEncodingTagged

#if !MIN_VERSION_base(4,16,0)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Un remove this?

src/Data/Aeson/Types/FromJSON.hs Outdated Show resolved Hide resolved
-> (ConName :* TypeName :* Options :* FromArgs arity a)
-> Object -> Parser (M1 i s f a)
recordParseJSONImpl mdef parseVal (cname :* tname :* opts :* fargs) obj =
handleMissingKey (M1 <$> mdef) $ do
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can we use explicitParseFieldOmit here?

* Adds 'omitField' to 'ToJSON'

* Adds 'omittedField' to 'FromJSON'
@@ -840,6 +877,10 @@ parseFieldMaybe = (.:?)
parseFieldMaybe' :: (FromJSON a) => Object -> Key -> Parser (Maybe a)
parseFieldMaybe' = (.:!)

-- | Function variant of '.:?='.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@since annotation, o nthis and omittedField2 etc.

@phadej phadej force-pushed the optional-fields branch 3 times, most recently from d835b9d to f0c4476 Compare June 15, 2023 14:54
- Add combinators for using omit* stuff in manually written instances
- Add Manual tests
- Cleanup OptionalFields.Common
- Fix TH and Generics
- Add combinators ToJSON1/2 and FromJSON1/2
- Const, Identity, Tagged and other newtypes
- Fix #687. ToJSON1 respects omitting fields
- Fix #571. Introduce allowOmittedFields to Generics/TH options.
- Resolve #792. () and Proxy can be omitted
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