-
Notifications
You must be signed in to change notification settings - Fork 23
CAD-4738 stm monad catch instance #16
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
Conversation
amesgen
left a comment
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.
Looks great, always cool to see when things get simpler over time 🎉
I only have a few minor comments below. As before, I did not commented on the IOSimPOR part as it is identical.
nfrisby
left a comment
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'll request some changes, just since I hope Yogesh finds a sufficiently lightweight way to address a couple of the eg testing comments. Nice stuff!
coot
left a comment
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.
It's on my todo list to review the PR... However, could you update ouroboros-network and check if all nightly tests are passing: for that one needs to add this snippet to cabal.project.local file:
package ouroboros-network-testing
flags: -nightly
And then run:
cabal run ouroboros-netowrk-framework:test && cabal run ouroboros-network:test
|
@coot Running the nightly tests now. I will update the results here soon. |
coot
left a comment
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.
Very nice, just a few remarks.
9d2e67f to
06b61ea
Compare
21f8b9e to
880fa47
Compare
d7d8d9f to
d073a5b
Compare
nfrisby
left a comment
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.
One last round of minor comment adjustments! 👍
nfrisby
left a comment
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 Requesting Changes one last time! I only caught spelling/grammar this time, but one of the typos is important.
Code looks great!
|
#16 (comment) is being addressed here. #35. So now the catch term in this code drop is |
bf7d862 to
a68ed16
Compare
nfrisby
left a comment
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.
All the comments have been addressed 👍
Except for the new one I'm opening right now---looks like this commit undoes a recently merged PR. That's why I'm Requesting Changes one last time.
a68ed16 to
a8da6e8
Compare
- Add support for Catch in IOSim and IOSimPOR - Add support for Catch in Test/STM.hs Co-authored-by: Marcin Szamotulski <[email protected]>
a8da6e8 to
c192e85
Compare
nfrisby
left a comment
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.
The diff looks minimal again. 🙌
Approved! Nice work 👍
Based on the branch "CAD-4750-generalise-orelse-frame", this commit introduces
Catchframe handler. It also adds Catch term in the reference STM for testing.Resolves IntersectMBO/ouroboros-network#1461