Bugfix/translate2 #1073
Bugfix/translate2 #1073
Conversation
Fixed issue with interrupting inner scope from interruptable parent.
- implemented first shot at resolving translate in algebra.
…F ~> G translation
| case Done(r) => translate0(fK, f(Right(r)), effect) | ||
| case Interrupted(_) => | ||
| translate0(fK, | ||
| f(Left(new Throwable("Translated scope cannot be interrupted"))), |
There was a problem hiding this comment.
Seems you forgot to properly implement interruption of translated scope here.
| } | ||
| } | ||
| } | ||
| // FreeC.Eval[Algebra[F, O, ?], Unit](OpenScope(s, interruptible)) |
There was a problem hiding this comment.
Commented out, remove?
|
|
||
| // Uncons is interrupted, fallback on `compileFoldLoop` has to be invoked | ||
| // The token is a scope from which we should recover. | ||
| case class Interrupted[X](token: Token) extends UR[X] |
There was a problem hiding this comment.
Can we move these classes to directly under Algebra to ensure they don't accidentally close over any state from this method?
There was a problem hiding this comment.
Oh I guess they close over F
There was a problem hiding this comment.
Well, there is unfortunatelly disrepance (see Continue) between ones used in compile and translate. In fact closing them inside F/G allowed me to discover another typecast bug.
Having said that, once we will battle-test this implementation, we could move them outside compile//translate
|
Looks great. Let's merge once you address Adam's comments and then I'll release RC2. |
|
ok shall be later today. so we shall be good to go fro RC2 tonite. Also could you pls test this against scodec before releasing? |
|
Yep already did and commented on the issue - all tests pass |
|
Great stuff! Should we ask for e.g. doobie to give this a try? Or release RC2 directly? |
+ addressed review comments
|
Testing doobie locally against this branch my only test errors are in postres and examples, which I believe are because I don't have a posgres instance up to test against. I don't know what tests were necessary to check, but all everything not requiring that appears to be passing on the series/0.5.x branch. |
|
@ChristopherDavenport Thanks very much for testing! @pchlupacek Ready for merge? |
|
I have now gotten the DB operation and 100% of doobie tests now passed. |
|
@ChristopherDavenport cool and thanks. @mpilquist yes, we could make a build. |
This fixes translate bug in M11/RC1 plus
@SystemFw @mpilquist thanks for comments