-
Notifications
You must be signed in to change notification settings - Fork 721
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
Use machine readable output for ToJSON ScriptWitnessIndex #5168
Use machine readable output for ToJSON ScriptWitnessIndex #5168
Conversation
29414ae
to
87a0591
Compare
87a0591
to
3f4815c
Compare
@@ -4129,6 +4130,29 @@ data ScriptWitnessIndex = | |||
| ScriptWitnessIndexWithdrawal !Word | |||
deriving (Eq, Ord, Show) | |||
|
|||
instance ToJSON ScriptWitnessIndex where | |||
toJSON = \case | |||
ScriptWitnessIndexTxIn n -> |
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.
Manual instances are cumbersome in maintenance and error-prone. Maybe genericToJSON
would be better here instead? For example:
data Foo = Foo !Text | Bar !Text | Baz !Text deriving (Generic)
genericToJSON (defaultOptions{sumEncoding=TaggedObject{tagFieldName="kind", contentsFieldName="value"}}) (Bar "barbar")
produces
{"kind":"Bar","value":"barbar"}
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'm not certain that buys enough to justify the cost for a number of reasons.
We need to do this for LogFormatting
and have no generic facility for that type class. If we have to manually type it out in one instance we may as well do it for both.
I use my editor's multi-cursor support and it takes very little time for something this regular.
My previous experience with aeson
generic generation was that the compile time was quadratic and with a large enough type that actually became a problem.
I'm not sure if this particular case is quadratic, but that the problem exists gives me sufficient hesitation.
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.
My main concern here is just an extra mental burden coming from the requirement to make proper changes in multiple places - what if you accidentally omit one place with your multi-cursor support? 😄
My previous experience with aeson generic generation was that the compile time was quadratic and with a large enough type that actually became a problem.
Fair point, and I agree that it matters here. I'm not insisting on using Generic
here, but it would be better to have more safety either by using automatic generation of instances, or testing them in property tests.
PS. I had to roll-back prod deployment in the past once, because of faulty, manually written FromJSON
instance, which was undetected for over a year, so I guess I'm a bit oversensitive on this topic. 😄
3f4815c
to
41da7b6
Compare
This might be worth doing a golden test for, so waiting on this input-output-hk/hedgehog-extras#31 |
41da7b6
to
988b868
Compare
988b868
to
a0e3505
Compare
Description
ToJSON ScriptWitnessIndex
instanceJSON
output uses this instance."ExtraRedeemers"
object contents are machine readable rather than difficult-to-parse user-friendly-string.Checklist
See Runnings tests for more details
CHANGELOG.md
for affected package.cabal
files are updatedhlint
. See.github/workflows/check-hlint.yml
to get thehlint
versionstylish-haskell
. See.github/workflows/stylish-haskell.yml
to get thestylish-haskell
versionghc-8.10.7
andghc-9.2.7
Note on CI
If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.