-
Notifications
You must be signed in to change notification settings - Fork 451
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
Transfer Cont from @nomisrev to arrow #2661
Conversation
internal class Eager(val token: Token, val shifted: Any?, val recover: (Any?) -> Any?) : | ||
ShiftCancellationException() { | ||
override fun toString(): String = "ShiftCancellationException($message)" | ||
} |
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 we should use the same trick here as we do for Effect
, so users have an escape hatch to differentiate between JobCanncellationException
and ShiftCanncelationException
when they have to in low-level niche use-cases.
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.
added tests in EagerEffect and Effect for low-level use-cases
@@ -0,0 +1,202 @@ | |||
# Semantics of Structured Concurrency + Effect<R, A> |
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.
This is not the documentation we want, this was part of the design process/documentation.
What we want instead is the README.MD
I'm thinking we should make it part of the KDoc on top of Effect
, and link the page to Effect
as well.
Currently, a lot of users have a hard time understanding the difference between Effect
and EagerEffect
and when or why they should be using either { }
vs either.eager { }
. This is currently completely undocumented inside arrow-continuations
/arrow.core.computations
.
I think we should add proper documentation in this PR before we merge, otherwise, it's going to be dropped to the bottom of the backlog again.
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've ported most of the Docs and still updating others, but the last commit can be reviewed
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.
Updated most places I was aware of, can I get another review
I removed |
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.
Great work @i-walker! Thanks for all the great effort moving this into Arrow 🥳 👏 👏 👏
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/continuations/EagerEffect.kt
Outdated
Show resolved
Hide resolved
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/continuations/Effect.kt
Outdated
Show resolved
Hide resolved
- title: arrow.core.continutions.Effect | ||
url: /apidocs/arrow-core/arrow.core.continuations/-effect/ |
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.
Not blocking this PR but I think we should put Effect
under extensions and data types
because it's something we want to promote.
It's the new implementation of Arrow's most powerful feature "computation blocks", and offers polymorphic properties we didn't have before. If we can somehow reference from the Effect
page that'd be awesome.
Not sure if we have to link EffectScope
, and EagerEffectScope
since those should be linked from the Effect
* EagerEffect
page.
We can put EagerEffect
alongside Effect
and get away with only 2 new entries on the page, instead of a new section with 4 sub-sections.
I can take a stab at this in a subsequent PR though. What do you think?
cc\ @arrow-kt/maintainers
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.
Yes sounds great too, it would be great if we could link EffectScope with EagerEffectScope and showing they cover similar signatures.
And rearranging it sounds super.
On that note now that we moved on from Data type + extension (aka implementation of a Type class) we might want to consider just having Data types and computations.
The few typeclasses we have left can also have a dedicated place, if necessary
Just as a note, though.
@@ -63,6 +63,7 @@ s [Coroutines Guide](https://kotlinlang.org/docs/coroutines-guide.html) on Kotli | |||
- [Pure & Referentially Transparent Functions]({{ '/fx/purity-and-referentially-transparent-functions/' | relative_url }}) | |||
- [Kotlin's Std Coroutines package]({{ '/fx/coroutines/' | relative_url }}) | |||
- [Why suspend over IO monad]({{ '/effects/io/' | relative_url }}) | |||
- [Semantics of Structured Concurrency and Effect]({{ '/arrow/core/continuations/' | relative_url }}) |
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.
Idem. Arrow Core functionality.
We have a document already concerning Arrow Fx Coroutines & Structured Concurrency, no?
If we don't need to create a ticket to write one and add it to this documentation page.
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 have this, but it doesn't go as deep as Semantics of Structured Concurrency and Effect
maybe we can create a ticket and I'd be happy to contribute it. Or help in whatever way
@@ -68,6 +68,7 @@ boilerplate and enable direct syntax including [monad comprehensions and computa | |||
- [Kotlin's Std Lib Guide](https://kotlinlang.org/api/latest/jvm/stdlib/) | |||
- [Pure & Referentially Transparent Functions]({{ '/fx/purity-and-referentially-transparent-functions/' | relative_url }}) | |||
- [Why suspend over IO monad]({{ '/effects/io/' | relative_url }}) | |||
- [Semantics of Structured Concurrency and Effect]({{ '/arrow/core/continuations/' | relative_url }}) |
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.
If we follow the changes suggested above, we should add Effect
and EagerEffect
to Extensions and data types
above.
Can be done in subsequent PR.
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.
yes we can leave it as a task in a ticket regarding how/ or what we should show on the site
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
…ntinuations/EagerEffect.kt Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
…ntinuations/Effect.kt Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
Thanks for extensive review @nomisRev, learnt a lot 🤗 |
Thanks to @nomisRev proposal 🙌🏾
I've slightly changed the names from
Cont
toEffect
andContEffect
toEffectScope
, e.g.:Feedback is welcome☺️