arrow
arrow copied to clipboard
arrow-kt
Feature/remove validated
- Removed
ValidatedFrom the project - Had several knit reports (want to discuss and fix)
Hey @nomisRev In this Pr I removed only the Validated and I also removed the example files that contained Validated examples. (also renamed the examples to be consecutive).
At this point I tried to run a grade build and I've got a lot of knit warnings and lately an error:
Execution failed for task ':knitCheck'.
> knitCheck task failed, see log for details (use '--info' for detailed log).
Run 'knit' task to write 23 missing/outdated files.
I just want to double check how to proceed in this case and how to fix it (as well for the checks that the actions are not passing that looks like a lot of them)
@Atternatt The easiest fix is to remove all the Validated examples from Knit and run gradle knit again
@Atternatt maybe this is a bit too much asking, but could this be based on #2784? I think we are working on removing many similar things (like computation blocks), and duplicating work would be a waste of resources
@Atternatt maybe this is a bit too much asking, but could this be based on #2784? I think we are working on removing many similar things (like computation blocks), and duplicating work would be a waste of resources
I was going to pick that feature but @nomisRev told me that you already picked it so we were discussing what I could do. Then we came to this that consisted of removing Validated from Arrow.
So far, I removed all the references in the project and the example files. Perhaps we could remove a common file with coss dependencies between Validated and Tuple?
@Atternatt on a second thought, just go on. My removal touches some parts, but merging should take care of it.
on the other hand, would you mind to help me understand what the build errors for the github actions are in order to fix them?
Sure thing! The first step would be to run gradle apiDump to update the API files and then commit and push. Since you are removing (lots) of functions the API Compatibility Validator screams at you… but this is the point of starting a new point release, right?
To fix the Knit problems, remove all the files in arrow-libs/core/arrow-core/src/jvmTest/kotlin/examples and run gradle knit to recreate the examples. Knit expects to find a file for every example, but since you are removing a few, it can no longer backreference them and fails.
while removing documentation refering to Validated I found something I'm not sure how to fix in arrow-site/docs/docs/patterns/errorhandling/README.md:229.
How should we update this example?
@Atternatt the error handling docs are IMO up for revamping after these changes.
We’ve also changed how to do error handling in Effect, and I hope to change all APIs accordingly. So that would mean this doc would be almost completely outdated. So feel free to remove, or ignore it for now.
got it @nomisRev If there are more fails on the build through GitHub actions actions I'll start removing these files
@Atternatt this is great work! As @nomisRev mentions, there's still the hard part of merging it with the current arrow2 branch (is it up to date with the latest main?); I could help with that from Monday on if you prefer to focus on more interesting stuff.
hello @nomisRev and @serras I've been out for a couple of days but I'm back.
The next step would be to remove all redundant traverse operations, and to cover the desired functionality with mapAccumulating but perhaps cleaner, and easier to manage in a separate PR. WDYT?
I was thinking exactly the same, creating separated PR's in order to have a clear work. The main p[point will be creating the mapAccumulating tests without forgetting the ones I removed for validated now
@Atternatt this is great work! As @nomisRev mentions, there's still the hard part of merging it with the current
arrow2branch (is it up to date with the latestmain?); I could help with that from Monday on if you prefer to focus on more interesting stuff.
Is up to date with arrow2 branch but not with main @serras
@nomisRev @serras What do you think about creating a typealias for Either<List<L>, R> that could be named Validated<L,R>? does it make sense?
Is up to date with
arrow2branch but not withmain@serras
Great! Then we can merge for the Arrow 2.c series
@Atternatt I think the perfect alias would be Either<NonEmptyList<E>, A>, with some extension methods to create a Left with a singleton list.
Is up to date with
arrow2branch but not withmain@serrasGreat! Then we can merge for the Arrow 2.c series
sorry @serras I was wring, I made a mistake and merged main to this branch instead of arrow2 🤦🏻
I think the perfect alias would be Either<NonEmptyList<E>, A>, with some extension methods to create a Left with a singleton list.
Is the idea to make everything work over NonEmptyList since we can apply Semigroup<Nel<A>> and have some methods to flatten E rather than have it work over Semigroup<E>?
ok so i added a bunch of changes to this PR regarding #2787 : 1 - Removing all traverse operations 2- Re-adding all tests #2819 3 - Changed zip implementation
But as it was accepted previously by @nomisRev I'm not sure if the actions ar ran again or not.
Disclaimer: as we also need to implement #2788 and the previous function was using traverse to make it work I set it as a TODO() (will add implementation but not sure where if in this PR or in a complimentary one to make it smaller (this already is huge)
@Atternatt, we have a large number of conflicts 😱 Should we fix these first, and run the tests before reviewing?
This PR proposes following changes:
Integrate Validated into Either
Based on some discussion on KotlinLang, and quite some confusion why there is both Validated and Either especially amongst beginners we wanted to explore merging the two. So a small recap between the difference.
Either<E, A> and Validated<E, A> model the same ADT, which is the disjunction between E and A. Either allows you to write happy-path code, and allows sequencing Either<E, A> values that depend on each-other using bind (Monad). Whilst Validated only allows combining values using zip, where you combine both (E, E) -> E and (A, B) -> C. (Applicative). The functionality of Validated can also be exposed over Either, and this PR proposes exactly that.
The reason for doing this is that we want to heavily decrease the API surface, and the resulting learning curve. After the 1.x.x series we realised that combing with the Arrow DSL having both types is not useful, and this is a good opportunity to heavily decrease the API surface. The 1.x.x Either API deprecation PR explains this a little bit as well.
Integrating Validated into Either
In 1.x.x we deprecated Either#zip in favour of either { f(bind(), b.bind(), ...) } which also solves the arity-n issue of zip. This PR replaces the Either#zip behaviour with the accumulating errors powers of Validated, but we can also consider a different name than zip such as fa.validate(fb, ::Tuple2) or fa.accumulate(fb, ::Tuple2). This PR proposes 3 different signatures to work with Either for validation purposes (only arity-2 shown below).
/** The most generic signature */
inline fun <E, A, B, C> Either<E, A>.zip(
combine: (E, E) -> E,
other: Either<E, B>,
transform: (A, B) -> C
): Either<E, C>
/** Automatically accumulate inside NonEmptyList, without requiring the original `E` to be wrapped */
inline fun <E, A, B, C> Either<E, A>.zip(
other: Either<E, B>,
transform: (A, B) -> C
): Either<NonEmptyList<E>, C>
/**
* A function to combine results of the above function, or validations that would previously return `ValidatedNel`.
* Regular `Either` values can be lifted into `NonEmptyList<E>` by using `toEitherNel()`.
*/
inline fun <E, A, B, C> Either<NonEmptyList<E>, A>.zip(
other: Either< NonEmptyList<E>, B>,
transform: (A, B) -> C
): Either<NonEmptyList<E>, C>
traverse
Besides combining independent values of Validated a second big use-case for using it is traverse which is completely being deprecated for the Arrow DSL, and therefore we also need to find a solution for where you'd previously use traverse + Validated. To replace traverse one can simply use the DSL + Iterable#map, and therefore we thought mapAccumulating offers us an idiomatic name for accumulating typed errors which is what traverse + Validated did.
either<String, List<Int>> {
listOf(1, 2, 3).map { raise("fail") }
//listOf(1, 2, 3).map { Either.Left("fail").bind() }
} // Either.Left("fail")
either<String, List<Int>> {
listOf(1, 2, 3).mapAccumulating(String::plus) { raise("fail") }
//listOf(1, 2, 3).mapAccumulating(String::plus) { Either.Left("fail").bind() }
} // Either.Left("failfailfail")
In the same fashion as zip mapAccumulating also provides 2 different methods.
public inline fun <Error, A, B> Iterable<A>.mapAccumulating(
semigroup: Semigroup<Error>,
transform: Raise<Error>.(A) -> B,
): Either<Error, List<B>>
public inline fun <Error, A, B> Iterable<A>.mapAccumulating(
transform: Raise<Error>.(A) -> B,
): Either<NonEmptyList<Error>, List<B>>
It works over Raise<E>, but returns a concrete Either. Snippet below compares traverse to mapAccumulating usages:
fun process(i: Int): Either<String, Int> = i.right()
listOf(1, 2, 3).traverse { process(it) }
listOf(1, 2, 3).mapAccumulating { process(it).bind() }
This API is optimised for context receivers, or working with Raise<E> extension functions. Otherwise it requires the additional call to bind().
Additional changes
Semigroup
To make this PR compile, and these functions to accept (E, E) -> E where fun interface Semigroup is defined the abstract function of Semigroup had to change shape from E.(E) -> E to (E, E) -> E. This is done in a non-binary breaking way, but we can still consider removing Semigroup in a different PR or changing its APIs.
Sounds like an overall positive change! We felt the pain of always reaching for ValidatedNel, but playing with types to the point of defining an EitherNel alias :D
My suggestions are:
-
mapAccumulating->mapAccumulatingLeft - I'd still keep the equivalent of
sequenceto flatten lists ofEither
Hey @pakoito, Thanks for sharing, great to hear that you've also experienced this and looked for a middle ground in 1.x.x 😁
mapAccumulating -> mapAccumulatingLeft
I felt mapAccumulating was already too long 😂 The name is definitely open for suggestions 🙏 I personally feel mapAccumulatingLeft is a bit long, but mapAccumLeft is perhaps a bit foreign. Should it reflect Raise, or did you choose Left in the name because of the Either return type? Perhaps collectRaise 🤔 collect conflicts a bit with Flow#collect though..
Do you like the new signatures?
I'd still keep the equivalent of sequence to flatten lists of Either
We can definitely do that, but sequence is perhaps not the best name for this 🤔 Any suggestions?
The convention in the stdlib is "XOrY", so this could follow:
For mapping: mapOrAccumulate, mapOrRaiseFirst
For sequencing, "merge" isn't taken: mergeOrRaiseFirst + mergeOrAccumulate.
Do you like the new signatures?
You know I do :D Nested zips will be a bit meh, but I'll probably reach for mergeOrAccumulate + deconstruction instead.
For mapping: mapOrAccumulate, mapOrRaiseFirst
mapOrRaiseFirst do you mean short-circuit behaviour? That is just map.
object Error
fun process(i: Int): Either<Error, Int> = i.right()
fun Raise<Error>.process2(i: Int): Int = i
either<Error, List<Int>> {
listOf(1, 2, 3).map { process(it).bind() }
listOf(1, 2, 3).map { process2(it) }
}
I do like mapOrAccumulate! It's more descriptive than mapAccumulate since with mapAccumulate the Accumulate seems to refer to happy-path whilst Or is a Kotlin Std established pattern that it refers to unhappy-path. Thanks @pakoito ❤️
For sequencing, "merge" isn't taken: mergeOrRaiseFirst + mergeOrAccumulate.
I was originally thinking of flatten but didn't propose it since there is no actually flattening (of List or otherwise) happening. Arrow already has Either<A, A>.merge, do you think it would be confusing to use merge here as well?
Nested zips will be a bit meh, but I'll probably reach for mergeOrAccumulate + deconstruction instead.
Do you mean nested zips to achieve arities higher than 9? mergeOrAccumulate + deconstruction is untyped 😭
Arrow already has Either<A, A>.merge, do you think it would be confusing to use merge here as well?
I'd rename that merge to converge or similar, if that frees the term. It's an extfun over a different type so it'll light up during migration.