Skip to content
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

Add attempt builder as an imperative alternative to recover #3340

Open
wants to merge 1 commit into
base: arrow-2
Choose a base branch
from

Conversation

kyay10
Copy link
Collaborator

@kyay10 kyay10 commented Dec 28, 2023

The goal of this builder is to allow early-returning the "happy path" and thus handling the "error path" in the code that follows.
I couldn't add this in main because it conflicted with a deprecated method, and so it wasn't useable inside of other Raise instances. Perhaps if there's a better name than attempt, then this change can be backported.

Copy link
Member

@serras serras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting addition!

@nomisRev
Copy link
Member

I am unsure about this addition. My immediate thoughts are that early returning is considered a code-smell by many, and I am not sure what additional patterns this enables?

If we want to easily allow transforming the error, which is what I see mostly here perhaps we need a variant of recover that re-raises the error? So an alias for recover({ foo.bind() }) { msg -> msg.toList() }. WDYT?

Comment on lines +512 to +514
val errorList = attempt { return block(RaiseAccumulate(this)) }
addErrors(errorList)
return EmptyValue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, here also really not fan of the early return 😅
I had to read this snippets too many times to understand what's going on :/

@kyay10
Copy link
Collaborator Author

kyay10 commented Jan 16, 2024

I understand that early returns are quite controversial. I think, however, that it can be really nice to use whenever the error path is more complicated than the happy path. For instance, trying out different methods until one succeeds becomes really easy (similar to ?:):

attempt { return easyToPerformTransformation() }
val error = attempt { return harderToPerformTransformationWithMoreChecks() }
attempt { return maybeCleanUpMalformedData(error) }
raise MyDomainError(malformedError = error)

Of course, this can be done with recover or withError etc, but attempt offers a way to do it linearly without extra nesting and in each step allowing inspection of the error.

Another place where I used attempt is in this code in arrow-context (simplified for brevity's sake):

context(Raise<NonEmptyList<Error>>)
internal inline fun <Error, A, B> Iterable<A>.mapOrAccumulate(
  transform: context(Raise<NonEmptyList<Error>>) (A) -> B
): List<A> {
  val iterator = iterator()
  val result = ArrayList<A>()
  val firstError = attempt {
    for (item in iterator) {
      result.add(transform(this, item))
    }
    return result
  }
  buildList {
    addAll(firstError)
    for (item in iterator) {
      attempt {
        transform(this, item)
        continue
      }.let(::addAll)
    }
  }.toNonEmptyListOrNull()!!.let(::raise)
}

The second attempt call could be replaced with a recover, but I think it's more imperative this way because it states that if everything succeeds, continue, else add the errors. If we replace the first attempt call with recover, the code quickly becomes deeply nested and harder to follow IMO.
I know that imperative programming with early returns, breaks, continues, etc is not common in Kotlin, but I think that it's a matter of style.
Perhaps it is too much a matter of style that only individual users should decide on introducing it themselves, though, but I'll leave that decision up to you.

@nomisRev nomisRev added the 2.0.0 Tickets / PRs belonging to Arrow 2.0 label Jan 17, 2024
@nomisRev
Copy link
Member

Okay, thank you for clarifying.

Perhaps it is too much a matter of style that only individual users should decide on introducing it themselves, though, but I'll leave that decision up to you.

Well, it's up to all of us together 😉 I added arrow 2.0 tag, so lets this sink in a bit while we focus on 1.2.2.

@serras
Copy link
Member

serras commented Jan 17, 2024

My 2 cents here is that attempt could be helpful for some style of programming. However, I don't think we should rewrite the rest of the code to use attempt, but rather keep that isolated. I also feel the code using early returns becomes harder to understand.

Copy link
Member

@serras serras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this form this can be included in Arrow; maybe not the style we want to promote, but useful in some cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0.0 Tickets / PRs belonging to Arrow 2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants