-
Notifications
You must be signed in to change notification settings - Fork 166
[DO NOT MERGE] Rename CDDL fields to better reflect their origin and be more reusable #5339
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
base: master
Are you sure you want to change the base?
Conversation
9594784 to
e2e3e15
Compare
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.
Apart from the potential irregularity I commented on, this refactoring seems like it should make no difference to the CDDL.
Perhaps the test failures are due to the issue I pointed out?
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 had a first pass and left some comments.
In addition, I'm also wondering if it's beneficial to remove the distinction between the types in cddl that represent hashes vs just bytes (so the replacement of hash32 with bytes32).
Even though they boil down to the same thing, it might bring some value to distinguish them? What do you think?
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Binary/CDDL.hs
Show resolved
Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Binary/CDDL.hs
Show resolved
Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Binary/CDDL.hs
Show resolved
Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Binary/CDDL.hs
Show resolved
Hide resolved
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 only took a quick look at this PR. I'll do a more thorough review tomorrow.
NOTE
This is deprecated in favour of potentially several new ones.
Description
This addresses the initial part of #5194 .
Follow-up PRs would be to actually address the heart of #5194 to reduce duplication to a minimum.
Checklist
CHANGELOG.mdfiles updated for packages with externally visible changes.NOTE: New section is never added with the code changes. (See RELEASING.md).
.cabalandCHANGELOG.mdfiles when necessary, according to theversioning process.
.cabalfiles updated when necessary.NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
scripts/fourmolize.sh).scripts/cabal-format.sh).scripts/gen-cddl.sh)hie.yamlupdated (usescripts/gen-hie.sh).