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

Raise accumulating/zip DSL #3436

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Raise accumulating/zip DSL #3436

wants to merge 8 commits into from

Conversation

serras
Copy link
Member

@serras serras commented May 25, 2024

This PR introduces a mini-DSLs for "zip" operations in Raise. The idea is to extend zipOrAccumulate to any amount of arguments, and make the code a bit more linear, by using the power of property delegation.

These are two tests that showcase the functionality: you use accumulating to mark those parts which should "add" to the errors, instead of merely using fail-first, and using delegation you don't need to remember anything else. If you use a raise outside an accumulating, all the errors accumulated until that moment are added to the other one.

@Test fun raiseAccumulatingTwoFailures() {
  eitherNel {
    val x by accumulating { raise("hello") ; 1 }
    val y by accumulating { raise("bye") ; 2 }
    x + y
  } shouldBe nonEmptyListOf("hello", "bye").left()
}

@Test fun raiseAccumulatingIntermediateRaise() {
  eitherNel {
    val x by accumulating { raise("hello") ; 1 }
    raise("hi")
    val y by accumulating { 2 }
    x + y
  } shouldBe nonEmptyListOf("hello", "hi").left()
}

@serras serras requested a review from nomisRev May 25, 2024 06:05
@serras serras changed the title Parallel DSLs Zip DSLs May 25, 2024
Copy link
Contributor

github-actions bot commented May 25, 2024

@serras serras changed the title Zip DSLs Raise accumulating/zip DSL May 25, 2024
Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

Love it! Amazing. I am trying to think of a better name than nel. accumulate might be an option, like Iterable.mapOrAccumlate and it fits with the SAM.

@serras
Copy link
Member Author

serras commented May 25, 2024

I've tried a few new names. I've also realized that every "runner" (either, result, ...) can give raise to a new accumulate, so I made that part generic. The question now is... what is the relation between this new form of accumulation and RaiseAccumulate?

@nomisRev
Copy link
Member

nomisRev commented May 25, 2024

I am willing to sink time into the IntelliJ plugin to provide a project wide refactor if you can help me get running. I am willing to do that for every breaking change we make between the last patch and 2.0.0!

So please review all APIs again in whatever way you think makes sense. If you see a good opportunity to split things of to smaller modules, that are like extremely tiny and make sense like AutoClose/ResourceScope/SagaScope which is a single type Scope which I pitched like 2 years ago. I am going to take another stab at that now. It's 3 the same thing, a finaliser typeclass.

EDIT: couldn't sleep, wrote some silly code. Closed it, will do it properly after resting 😅

@nomisRev
Copy link
Member

After thinking about this a bit more, the delegation is what forces to first split into a declaration, right? To prevent:

Person(
   accumulating { ensureNotNull(name) { "Null name" } },
   accumulating { ensure(age => 18) { "Not an adult" } },
)

That is not possible with proper accumulation, right? So, the following test case.

@Test fun raiseAccumulatingTwoFailures() {
  accumulate {
    val x by accumulating { raise("hello") ; 1 }
    val y by accumulating { raise("bye") ; 2 }
    val xy = x + y
    val z by accumulating { raise("BOOM"); 3 }
    xy + z
  } shouldBe nonEmptyListOf("hello", "bye").left()
}

@serras
Copy link
Member Author

serras commented May 26, 2024

About accumulation, yes, the delegated property trick is what makes it "wait" until the moment the value is actually needed for the next step, while accumulating the rest.

In the last commit I've merged RaiseAccumulate with this new idea, and I think it gives a wonderful DSL for error accumulation. Many of the current operations become now combinations of accumulate, accumulating, and bindOrAccumulate.

@serras serras marked this pull request as ready for review May 26, 2024 17:25
@serras serras requested a review from kyay10 May 26, 2024 17:25
Comment on lines 800 to 801
public abstract val result: A
public operator fun getValue(value: Nothing?, property: KProperty<*>): A = result
Copy link
Member

Choose a reason for hiding this comment

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

Do all these need to be public?

Copy link
Member Author

Choose a reason for hiding this comment

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

getValue has to be public for by to work. I added value just to allow using the Value on its own (some people don't like delegation), but it can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

I would initially hide whatever is possible, we can always make it public afterwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@CLOVIS-AI
Copy link
Contributor

This looks very nice! I played around on my own trying to get something like this to work, but I couldn't get anything I was happy with. It's great that we're finally going to be avoid multi-lambda syntax and the n-arity duplications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants