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

[PROPOSAL] Ior API deprecation, and preparation for 2.x.x #2928

Merged
merged 36 commits into from
Mar 10, 2023
Merged

Conversation

gutiory
Copy link
Collaborator

@gutiory gutiory commented Feb 10, 2023

Ior API deprecation

This PR proposes a smaller API surface for Ior without compromising its functionality or feature set.
The goal of these changes is to offer APIs that are more powerful and more idiomatic to use in Kotlin. Following the signatures and naming of some other well-known APIs from Kotlin Std, or KotlinX Coroutines

Ior (API Size 65 -> 28)

(Bi)Functor / Applicative / Monad (API Size 22 -> 5)

Keep

  • flatMap: transform right value. with Ior
  • flatten: flatten nested Iors.
  • map: transform right value, when Right or Both
  • widen: change generic parameter to a super class type.

Remove

  • exists -> fold({ false }, predicate) // map(predicate).getOrElse { false }
  • leftWiden: widen already covers Left , Right , and Both
  • lift(f): replace with { it.map(f) }
  • lift(fa, fb): replace with { it.map(fb).mapLeft(fa) }
  • bimap: replace with map(fb).mapLeft(fa)
  • findOrNull: use getOrNull()?.takeIf(predicate)
  • void: use map { }
  • zip -> Replaced with Validated behavior, original zip will be inline ior + bind

Applicative/MonadError (API Size 4 -> 4)

Keep

  • getOrElse: get Right value if Right or Both . Get default if Left
  • mapLeft: transform left value, when Left or Both
  • replicate: Hard to find a simple sugestion to replace the use

Foldable / (Bi)Traverse (API Size 25 -> 1)

Keep

  • fold

Remove

All traverse, bitraverse, sequence, bisequence, crosswalk are set to be replace with fold

  • all
  • bicrosswalk
  • bicrosswalkMap
  • bicrosswalkNull
  • bitraverse
  • bitraverseEither
  • bitraverseNullable
  • bitraverseOption
  • bitraverseValidated
  • bifoldLeft: replace with fold
  • bifoldMap: replace with fold
  • crosswalk
  • crosswalkMap
  • crosswalkNull
  • foldLeft: replace with fold
  • foldMap: replace with fold
  • traverse( iterable )
  • traverseEither
  • traverse( Either )
  • traverseNullable
  • traverse( Option )
  • traverseOption( Option )
  • traverse(Validated)
  • traverseValidated(Validated)

Utilities (14 -> 9)

Keep

  • bothIor: construct a Both from a Pair
  • getOrNull: get Right if Ior is Right or Both.
  • leftOrNull: get Left if Ior is Left or Both.
  • leftIor: construct a Left from a value
  • padNull: return the Ior as a Pair o nullables.
  • rightIor: construct a Right from a value
  • swap: flip type arguments
  • toEither: return Right if the Ior is Right or Both. Left otherwise.
  • unwrap: return the isomorphic Either of the Ior

Remove

  • combine -> Will be covered by accumulating zip behavior in 2.x.x
  • isEmpty: use isLeft
  • isNotEmpty: use isRight
  • orNull: use getOrNull
  • toValidated: Validated removed in Arrow 2.x.x

@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2023

Kover Report

File Coverage [59.78%]
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/Ior.kt 59.78%
Total Project Coverage 44.10%

@gutiory gutiory marked this pull request as ready for review February 10, 2023 10:25
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.

Great work @gutiory 🙌 Thank you so much for looking into this! I added a bunch of suggestions 😅 Feel free to apply them, or use them as a guide to update the PR. Whatever works best for you.

  • ReplaceWith is missing imports, added Ior. in most cases to avoid conflicts with Either.Left & Either.Right.
  • A bunch of identity calls slipped in due to refactoring tools of IDEA.
  • Some styling nits (Spaces before { mostly).
  • Small improvements to foldLeft and bifold, empty + a == a for Monoid so the ReplaceWith can be simplified.

}
}

@Deprecated(
NicheAPI + "Prefer when or fold instead",
ReplaceWith("fold({ f(c, it) }, { g(c, it) }, { a, b -> g(f(c, a), b) })")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ReplaceWith("fold({ f(c, it) }, { g(c, it) }, { a, b -> g(f(c, a), b) })")
ReplaceWith("this.fold<C>({ f(c, it) }, { g(c, it) }, { a, b -> g(f(c, a), b) })")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

question: Why is it needed here the this.fold<C> part?. I've tried a couple of replacement and the result is the same than using the current solution.

This comment follows the conventionalcomments.org standard

Copy link
Member

Choose a reason for hiding this comment

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

This replacement was not working for me as is :/
Just adding this is sufficient, it's strange it works for foldMap, without this.

So the <C> can be ignored, IIRC adding it adds a type-check during replacement that checks if the resulting replacement is correct in cases where it otherwise is incorrect but it's not entirely clear to me when it's needed and when not. Similar in this case for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

gutiory and others added 8 commits February 14, 2023 11:53
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
Alternative methods with `predicate` function parameter is added for every method.
`isNotEmpty()` replacement was incorrect.
…ther`

`Both` is prepend with `Ior.` as well, to keep communality within the code.
Missing imports are added in the `ReplaceWith` class.
@gutiory gutiory changed the title [PROPOSAL] Iop API deprecation, and preparation for 2.x.x [PROPOSAL] Ior API deprecation, and preparation for 2.x.x Feb 21, 2023
@nomisRev nomisRev mentioned this pull request Feb 24, 2023
14 tasks
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.

Sorry to keep you waiting for so long @gutiory. Thanks for all the great work so far 🙌
Couple of things we still need to address, that I missed in the first review:

  1. We missed a couple of deprecations:
  • isRight, isLeft, & isBoth properties
  • toValidated
  • bitraverseValidated, bisequenceValidated
  • replicate
  1. we should align getOrElse function with the other signatures. This can be done without deprecation, in a source -and binary compatible way. Either.getOrElse { }, Either.getOrElse { _ -> }.

  2. We need to introduce getLeftOrNull similarly to getOrNull and deprecate the current leftOrNull method. Similarly I would love to have a better name for padNull but getBothOrNull seems wrong since it returns Pair<A?, B?> 🤔

I've tested a couple of the ReplaceWith, and left comments. Also resolved some of the previous comments that were still open.

arrow-libs/core/arrow-core/api/arrow-core.api Outdated Show resolved Hide resolved
}
}

@Deprecated(
NicheAPI + "Prefer when or fold instead",
ReplaceWith("fold({ f(c, it) }, { g(c, it) }, { a, b -> g(f(c, a), b) })")
Copy link
Member

Choose a reason for hiding this comment

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

This replacement was not working for me as is :/
Just adding this is sufficient, it's strange it works for foldMap, without this.

So the <C> can be ignored, IIRC adding it adds a type-check during replacement that checks if the resulting replacement is correct in cases where it otherwise is incorrect but it's not entirely clear to me when it's needed and when not. Similar in this case for this.

All vals must still be public and the `JVM` name of the respective methods must differ due to a clash when generating the apiDump
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.

The PR looks really great! Thank you for all the hard work, and a lot of patience @gutiory. 🙏 👏 👏 👏
Let's merge this this 🙌

@Deprecated(
"leftOrNull is being renamed to getOrNull to be more consistent with the Kotlin Standard Library naming",
ReplaceWith("getLeftOrNull()")
)
public fun leftOrNull(): A? {
Copy link
Member

Choose a reason for hiding this comment

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

Thank you, to completely track decision making on this. Result from Kotlin Std has exceptionOrNull alongside getOrNull instead of getExceptionOrNull so we're following the same API naming pattern.

@nomisRev nomisRev requested review from serras, franciscodr and a team March 8, 2023 16:50
Copy link
Collaborator

@franciscodr franciscodr left a comment

Choose a reason for hiding this comment

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

Great job, @gutiory!

@gutiory gutiory merged commit 55d441c into main Mar 10, 2023
@gutiory gutiory deleted the jg-ior-API branch March 10, 2023 12:19
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.

3 participants