-
Notifications
You must be signed in to change notification settings - Fork 449
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
fix bind logic for Option to raise the transformed value. #2948
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hey @freynder,
We spoke and Slack and I am afraid I was too fast to respond 🙈 My apologies.
Looking at this signature, with this implementation I am actually wondering if it makes sense at all. So first of all thank you to raise this PR, and thoughts on the signature 🙏
I am going to rubber duck my thoughts, and ask some feedback from some fellow contributors at the same time.
The lambda
Raise<R>.() -> Aallows to bind, or unwrap,Optionand in the caseNoneis encountered it will invoke the lambda. The lambda allows to raise a value ofRinto the outerinterface Raise<R>, or fallback to a value of A.Raise<R>lambda receiver is redundant because we're already inside the context ofinterface Raise<R>, so even without the lambda receiver we can callraise(r).Raise<R>, we have() -> Awhich matches the already existingOptionAPI ofgetOrElse.interface, it cannot beinlineso it's inferior to the already existinggetOrElsewhich is inline and thus allows forsuspendto pass through.So this API can be completely replaced by
getOrElseand simplifies the examples of the KDoc above.suspend fun test() { val empty: Option<Int> = None either { - val x: Int = empty.bind { _: None -> 1 } + val x: Int = empty.getOrElse { 1 } - val y: Int = empty.bind { _: None -> raise("Something bad happened: Boom!") } + val y: Int = empty.getOrElse { raise("Something bad happened: Boom!") } - val z: Int = empty.recover { _: None -> + val z: Int = empty.getOrElse { delay(10) 1 - }.bind { raise("Something bad happened: Boom!") } + } x + y + z } shouldBe Either.Left("Something bad happened: Boom!") }As the example shows we can now call
suspend fun delaydirectly into thegetOrElselambda, and we can also still callraise& co from inside thegetOrElselambda.The same applies for
Result#bind. Since none of this code has yet been released, we can still remove these APIs in favour of the simplerinlinemethods on the original APIs. Fixing this issue also requires some other small changes in Builders.kt.If you're interested @freynder I'd be happy to guide you through all the changes☺️ We can discuss further on Slack, or here.
cc\ @serras @franciscodr @raulraja
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.
Hello @nomisRev,
No problem, I hadn't looked at this from an API perspective, just wanted to make it consistent with the Either.bind() and others, they also do raise on error.
I fully agree with your assessment though, it would make the API much simpler. My only concern is that the bind() functions provided a consistent way for all monads to raise (and thus short-circuit) the program. getOrElse makes the raise call optional. I don't have enough knowledge currently to determine if that's important though.
Thank you for your great feedback, very much appreciated.
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.
Hello,
Looking further at the code, I also noticed the same reasoning related to your point "1." can be applied to Raise.catch:
Here the catch function receiver also seems redundant as it does not need the Raise receiver since it can access it from the this@catch Raise scope.
A counter argument to this may be that the caller may want to use a predefined lambda variable as a handler for the catch function with receiver to receive the context. Not sure how common that would be though, personally I would prefer to be explicit.
Probably also same remark for recover
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.
Hey @freynder,
This is great feedback and I was actually also looking at this. There is multiple benefits to this actually, and we should review the current signatures in
arrow.core.raise.*to see how we can simplify them.Looking for example at
recover:Besides still supporting the current use-cases it also makes this function more general purpose, because you can now use it to execute
Raise<E>based programs and resolve the error completely without having to stick toRaise<E>.So thanks again for raising this issue, and PR. I think it will result in another significant improvement, and simplification of this package.
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.
Hey @nomisRev,
Wow, that indeed removes the
Raise<R>receiver completely. Loving how elegant this is implemented with the fold function actually creating an isolated raise context and using the general callbacks to transform / act on the possible outcomes.Looking at it from the above perspective, recover and catch are basically predefined behaviors for fold. That makes the mental model much easier for me.
Thank you!
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.
Hey @freynder,
That is absolutely correct. I might steal that phrasing, and those bullet points for the new documentation 😁
I am going to close this PR, since it's preceded by #2954.
Thank you again for triggering this simplification, and discussion! Glad it helped you get a better mental model of this encoding as well.