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

rename Sequence sequenceX, traverseX and deprecate Sequence.some #2686

Merged
merged 11 commits into from Mar 21, 2022

Conversation

i-walker
Copy link
Member

@i-walker i-walker commented Mar 10, 2022

Rename to either sequence or traverse:

  • sequenceEither
  • sequenceValidated
  • sequenceOption
  • traverseEither
  • traverseOption
  • traverseValidated

Deprecate:

  • some

@i-walker i-walker changed the title deprecate Sequence terminal operators deprecate Sequence terminal operators and legacy api Mar 10, 2022
@i-walker i-walker requested a review from nomisRev March 10, 2022 14:35
@@ -612,15 +612,19 @@ public fun <A, B> Sequence<Validated<A, B>>.separateValidated(): Pair<Sequence<A
return asep to bsep
}

@Deprecated("Terminal operators on Sequence are being deprecated. Please use either one of the supported terminal operators of Sequence and opt for supported [traverse] functions")
Copy link
Member

@raulraja raulraja Mar 11, 2022

Choose a reason for hiding this comment

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

What is the replacement that a user would perform in this case? The current message is a bit cryptic. If we know the expression that would replace sequenceeEither then we can add it to a ReplaceWith.

Copy link
Member Author

Choose a reason for hiding this comment

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

here a user could opt to use toList().sequenceEither() instead, if that sounds good to all, I can add it?

Copy link
Member

Choose a reason for hiding this comment

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

There are several options here:

  • Deprecate and change the signature to Sequence<Either<E, A>>.sequenceEither(): Either<E, List<A>>, which doesn't lie about the fact that it's terminal. This would be more optimized than toList().sequenceEither().

  • Deprecate and ReplaceWith toList().sequenceEither().

Now that I think about it, the latter might be problematic since that cannot short-circuit an infinite Sequence on Either.Left but deprecating towards a List in the return type is going to be very difficult without breaking the Kotlin API (the binary can be kept in tact by using HIDDEN).

@i-walker i-walker changed the title deprecate Sequence terminal operators and legacy api rename Sequence sequenceX, traverseX and deprecate Sequence.some Mar 17, 2022
…quence.kt

Co-authored-by: Imran Malic Settuba <46971368+i-walker@users.noreply.github.com>
@nomisRev nomisRev requested review from pakoito and a team March 21, 2022 08:19
Copy link
Member

@raulraja raulraja left a comment

Choose a reason for hiding this comment

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

It looks excellent @i-walker, thanks. Since we are renaming sequenceX to sequence, is there a better name for this operation?. I feel in Kotlin this operation may be confusing by name, and you'd think you'd get a Sequence back instead of traversing over identity.

@i-walker
Copy link
Member Author

Since sequence == traverse(::identity) we could introduce f to have a default value ::identity for 2.0. Then we could drop sequence since the Traverse type class doesn't exist anymore
wdyt @raulraja ?

@i-walker i-walker merged commit 128bc08 into main Mar 21, 2022
@i-walker i-walker deleted the is-sequence-deprecation branch March 21, 2022 10:34
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