-
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
Documented the Twiddler
module
#3073
Conversation
fe06283
to
c30043a
Compare
ff9e55f
to
e6d9eb5
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.
Minor suggestions
libs/cardano-ledger-binary/test-lib/Test/Cardano/Ledger/Binary/Twiddle.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-binary/test-lib/Test/Cardano/Ledger/Binary/Twiddle.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-binary/test-lib/Test/Cardano/Ledger/Binary/Twiddle.hs
Outdated
Show resolved
Hide resolved
@Soupstraw FYI I didn't appprove the PR, since it is still marked as "draft". I wasn't sure if you plan to add more to this PR. Also ormolu CI fails |
Yes, I figured I'll add some tests for the |
ac3dd05
to
e0ca2dc
Compare
a01a716
to
5dd714f
Compare
It's failing the probabilistic tests again.. |
Twiddler
module.Twiddler
module
libs/cardano-ledger-binary/test-lib/Test/Cardano/Ledger/Binary/Twiddle.hs
Show resolved
Hide resolved
Great documentation, I finally I understand the Twiddler :D |
libs/cardano-ledger-binary/test-lib/Test/Cardano/Ledger/Binary/Twiddle.hs
Outdated
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 agree with @teodanciu , this looks great! I also really love the new tests!!
5dd714f
to
a998778
Compare
a998778
to
44c270e
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.
Look great. However, this PR was suppose to be about documentation, right? 😉 Adding actual property tests should be done in a separate PR.
49ef3d8
to
f48bca0
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.
👍
f48bca0
to
b551bd7
Compare
This PR adds documentation to the
Twiddle
module.I also devised a law for
Twiddle
instances and wrote some tests that test that law.closes #3067