feat: Use PrimerJSON for most remaining JSON serializations.#604
Conversation
| newtype NameCounter = NC Natural | ||
| deriving (Eq, Enum, Show, ToJSON, FromJSON) | ||
| deriving (Eq, Enum, Show, Generic, Data) | ||
| deriving (FromJSON, ToJSON) via PrimerJSON NameCounter |
There was a problem hiding this comment.
I think this will change the serialisation, switching from newtype-deriving to generic-deriving. This probably means that openapi.json and the actual api return are out of sync (either before this change or after, I'm not sure!). However, I'm shortly going to be looking at this area again (#607), so am not too worried if it is broken for now.
There was a problem hiding this comment.
I did wonder whether we should newtype-derive this, but figured this isn't the time to change it. I missed that we were already using the newtype and this PR actually changes away from that (at a glance, I'd thought it was using anyclass-deriving).
There was a problem hiding this comment.
This was an odd one because the other newtype-derived cases are written as deriving newtype (FromJSON, ToJSON), so why is this different?
There was a problem hiding this comment.
I expect there's no particular reason for it, just an oversight. Deriving strategy annotations are optional.
There was a problem hiding this comment.
OK, in that case, I'll switch back to newtype-deriving for this one.
There was a problem hiding this comment.
Ach, hang on: I think I may have misled us here. The GHC user guide says "In case you try to derive some class on a newtype, and GeneralizedNewtypeDeriving is also on, DeriveAnyClass takes precedence." (see https://downloads.haskell.org/~ghc/latest/docs/html/users_guide/exts/derive_any_class.html)
I'm not sure which extensions are enabled in this file...
There was a problem hiding this comment.
As of the latest commit for this PR, it now reads deriving newtype (FromJSON, ToJSON). That's the same as everywhere else we derive using newtype so assuming we do want to elide the newtype wrapper in the serialized JSON (and I think we do), that's the correct deriving statement now, yes?
There was a problem hiding this comment.
Looking in more detail, this file is using GHC2021 and no other relevant extensions, so I think it has GND enabled but not DeriveAnyClass. I believe my original comment is correct (though maybe my confusion here is a sign that we should use strategy annotations more consistently!).
There was a problem hiding this comment.
As of the latest commit for this PR, it now reads
deriving newtype (FromJSON, ToJSON). That's the same as everywhere else we derive usingnewtypeso assuming we do want to elide thenewtypewrapper in the serialized JSON (and I think we do), that's the correctderivingstatement now, yes?
Yes, I think that is both what we want, and is consistent with the state before this PR.
dhess
force-pushed
the
dhess/primerjson-all-the-things
branch
from
18730e1 to
09a4e24
Compare
3 participants
No description provided.