-
Notifications
You must be signed in to change notification settings - Fork 156
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
Move functionality of Cardano.Ledger.Pretty to Test.Cardano.Ledger.Generic.PrettyCore #3973
Conversation
f35319d
to
5f0104a
Compare
removed all reference to Cardano.Ledger.Pretty in Test.Cardano.Ledger
5f0104a
to
21f869e
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.
Nice. Next step is to remove cardano-ledger-pretty
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 think you forgot to bump the version of cardano-ledger-test here. This is probably why the latest version on CHaP is dated back to the summer 2023?
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 see, it's set to 9.9.9.9
and "not supposed to be released". Why is that the case @lehins? Though it's a testing library, prettyprinting stuff is something others may want to use.
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.
We do not want to provide custom pretty printing functionality for public use, because it comes with a lot of overhead for the ledger teqm. Also, "others may want to use" is not good enough reason for us to provide this functionality in a public library. Quick search through github revealed that noone uses that functionality, so we decided to move it into the test suite.
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.
Thank you for the reply. We are using it in the new version of the Cardano test emulator we are working on. It sounds like you recommend not using it all, but I don't see why we might want to roll out our own version from scratch. BTW the older versions of that cardano-ledger-test
are in CHaP, which might be misleading. Technically it's public either way, it's just a matter of convenience. Not only prettyprinting stuff from the test library is useful for our project. And it's not a big deal to use it directly, though I would prefer having it in CHaP with proper version bumps when they happen. Anyway, thanks for your hard work on the ledger.
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.
BTW the older versions of that cardano-ledger-test are in CHaP, which might be misleading.
I know. CHaP is immutable, so we can't remove it. Initial version of cardano-ledger-test
was released early on by mistake.
Technically it's public either way, it's just a matter of convenience.
Not exactly. Providing official support for any functionality puts extra overhead on the ledger team, including testing, releasing, etc of that functionality. For example bunch of current pretty printers that are used for testing are very much incomplete, which is not a problem for us, but it would be a big problem for us if we maintained a library that provided this incomplete functionality. Like we did in the past.
And it's not a big deal to use it directly
I would advise against it, since we do not guarantee its correctness, use it at your own risk.
I'd suggest downstream users of ledger roll out their own pretty printers. If you are willing to maintain a library with pretty printers, which would become popular in the community, then we'd be happy to add such a library to CHaPs.
First step in the process of removing cardano-ledger-pretty
Fixes #3974
Fixes #3926
Description
Checklist
.cabal
andCHANGELOG.md
files according to theversioning process.
.cabal
files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)CHANGELOG.md
for the affected packages. New section is never added with the code changes. (See RELEASING.md)fourmolu
(usescripts/fourmolize.sh
)scripts/cabal-format.sh
)hie.yaml
has been updated (usescripts/gen-hie.sh
)