Skip to content

CAD-4738 stm monad catch instance#16

Merged
yogeshsajanikar merged 1 commit into
mainfrom
CAD-4738-stm-monad-catch-instance
Nov 12, 2022
Merged

CAD-4738 stm monad catch instance#16
yogeshsajanikar merged 1 commit into
mainfrom
CAD-4738-stm-monad-catch-instance

Conversation

@yogeshsajanikar
Copy link
Copy Markdown
Contributor

@yogeshsajanikar yogeshsajanikar commented Aug 2, 2022 •
edited by coot
Loading

Based on the branch "CAD-4750-generalise-orelse-frame", this commit introduces Catch frame handler. It also adds Catch term in the reference STM for testing.

Resolves IntersectMBO/ouroboros-network#1461

This was referenced Aug 2, 2022
Closed
Merged
amesgen
amesgen reviewed Aug 2, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@amesgen amesgen left a 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.

Comment thread io-sim/test/Test/STM.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/test/Test/IOSim.hs Outdated
Comment thread io-sim/test/Test/STM.hs Outdated
Comment thread io-sim/test/Test/STM.hs Outdated
Comment thread io-sim/test/Test/STM.hs Outdated
Comment thread io-sim/test/Test/STM.hs Outdated
nfrisby
nfrisby suggested changes Aug 4, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@nfrisby nfrisby left a 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!

Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated
Comment thread io-sim/test/Test/STM.hs
Comment thread io-sim/test/Test/STM.hs
Comment thread io-sim/test/Test/STM.hs
coot
coot reviewed Aug 5, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@coot coot left a 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

@yogeshsajanikar
Copy link
Copy Markdown
Contributor Author

yogeshsajanikar commented Aug 10, 2022

@coot Running the nightly tests now. I will update the results here soon.

@yogeshsajanikar
Copy link
Copy Markdown
Contributor Author

yogeshsajanikar commented Aug 10, 2022

@coot The tests that you mentioned pass!

Running tests in ouroboros-network with io-sim commit id 9d2e67f

cabal run ouroboros-network-framework:test -> All 47 tests passed (118.51s)
cabal run ouroboros-network:test -> All 314 tests passed (480.21s)

coot
coot reviewed Aug 18, 2022
View reviewed changes
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
coot
coot requested changes Aug 18, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@coot coot left a 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.

Comment thread io-sim/test/Test/STM.hs
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
@yogeshsajanikar yogeshsajanikar force-pushed the CAD-4738-stm-monad-catch-instance branch from 9d2e67f to 06b61ea Compare August 29, 2022 05:59
@yogeshsajanikar yogeshsajanikar marked this pull request as ready for review August 29, 2022 06:01
coot
coot reviewed Sep 22, 2022
View reviewed changes
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs Outdated
@coot coot changed the title Cad 4738 stm monad catch instance CAD-4738 stm monad catch instance Sep 23, 2022
@yogeshsajanikar yogeshsajanikar mentioned this pull request Oct 11, 2022
Open
1 task
@yogeshsajanikar yogeshsajanikar force-pushed the CAD-4738-stm-monad-catch-instance branch 2 times, most recently from 21f8b9e to 880fa47 Compare October 12, 2022 14:32
coot
coot reviewed Oct 13, 2022
View reviewed changes
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
@yogeshsajanikar yogeshsajanikar force-pushed the CAD-4738-stm-monad-catch-instance branch from d7d8d9f to d073a5b Compare October 13, 2022 18:06
@coot coot mentioned this pull request Oct 26, 2022
Closed
nfrisby
nfrisby suggested changes Oct 31, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@nfrisby nfrisby left a 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! 👍

Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Internal.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/test/Test/STM.hs
Comment thread io-sim/test/Test/STM.hs
Comment thread io-sim/test/Test/STM.hs Outdated
nfrisby
nfrisby reviewed Nov 8, 2022
View reviewed changes
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/src/Control/Monad/IOSim/Types.hs Outdated
Comment thread io-sim/test/Test/STM.hs Outdated
nfrisby
nfrisby suggested changes Nov 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@nfrisby nfrisby left a 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!

@yogeshsajanikar
Copy link
Copy Markdown
Contributor Author

yogeshsajanikar commented Nov 9, 2022 •
edited
Loading

#16 (comment) is being addressed here. #35. So now the catch term in this code drop is Catch :: Term t -> Term t -> Term t. The #35 handles exception. It also adds another exception type so that an exception can bubble up if the exception type is not handled. It is a WIP.

@yogeshsajanikar yogeshsajanikar force-pushed the CAD-4738-stm-monad-catch-instance branch from bf7d862 to a68ed16 Compare November 9, 2022 16:22
nfrisby
nfrisby suggested changes Nov 10, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@nfrisby nfrisby left a 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.

Comment thread io-sim/src/Control/Monad/IOSim/STM.hs Outdated
@yogeshsajanikar yogeshsajanikar force-pushed the CAD-4738-stm-monad-catch-instance branch from a68ed16 to a8da6e8 Compare November 10, 2022 07:08
@yogeshsajanikar @coot
Add support for MonadCatch
c192e85 
- Add support for Catch in IOSim and IOSimPOR
- Add support for Catch in Test/STM.hs

Co-authored-by: Marcin Szamotulski <coot@coot.me>
@yogeshsajanikar yogeshsajanikar force-pushed the CAD-4738-stm-monad-catch-instance branch from a8da6e8 to c192e85 Compare November 11, 2022 11:29
yogeshsajanikar
yogeshsajanikar commented Nov 11, 2022
View reviewed changes
Comment thread io-sim/src/Control/Monad/IOSimPOR/Internal.hs
nfrisby
nfrisby approved these changes Nov 11, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@nfrisby nfrisby left a 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 👍

@yogeshsajanikar yogeshsajanikar merged commit 9650e14 into main Nov 12, 2022
@yogeshsajanikar yogeshsajanikar deleted the CAD-4738-stm-monad-catch-instance branch November 12, 2022 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coot coot coot approved these changes

@dnadales dnadales Awaiting requested review from dnadales

@bartfrenk bartfrenk Awaiting requested review from bartfrenk

+2 more reviewers

@nfrisby nfrisby nfrisby approved these changes

@amesgen amesgen amesgen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add MonadCatch instance for IOSim's STM

4 participants

@yogeshsajanikar @coot @nfrisby @amesgen