search

Kotlin / kotlinx.coroutines

Library support for Kotlin coroutines
Apache License 2.0
13.05k stars 1.85k forks source link

Flow.completeAfter { it.isTheLastOne() } #3299

Open joffrey-bion opened 2 years ago

joffrey-bion commented 2 years ago

I have noticed the answer on https://github.com/Kotlin/kotlinx.coroutines/issues/2042 and read the concerns expressed in https://github.com/Kotlin/kotlinx.coroutines/issues/2065 which is presented as a solution to this problem.

While I concur with the naming challenges and the fact that we cannot use the name takeUntil in Kotlin for this purpose, I do believe the use case of ensuring a flow is finite / completes is quite important and prevalent especially since the addition of StateFlow and SharedFlow, and is worth a dedicated operator.

transformWhile indeed does the job, but it really doesn't convey the message cleanly, which after all is why I personally love Kotlin's collection and flow operators. takeWhile, drop(n), etc. really read well, but transformWhile { emit(it); it.isNotTheLastOne() } honestly doesn't. In particular it's unclear whether the element will be emitted if we return false (of course it is, since we emit before reaching the condition, but what I mean is that it's not obvious). The point is that the goal of this snippet doesn't pop right away at first glance. The code seems too complex to express a single idea - we're doing multiple things.

Since the use case at hand is about ensuring flow completion on a certain condition, I would like to suggest completeAfter { it.isTheLastOne() } as a possible name for this function. The name would suggest that we make the flow "complete" when a given condition is met, while still suggesting the element matching the condition will be included.

I understand that it doesn't follow the take convention, but it's actually not what we want to express here. The focus is not on the fact that all elements match a condition and we want to take them. The focus is about completing the flow after we see a particular sentinel value.

Another possible option would be dropAfter, which would match other helpers like substringAfter, and follow the drop naming convention for cutting off the end of a flow or collection. It would be slightly more general-sounding than completeAfter, but with less emphasis on the "completion" aspect - making the flow finite.

If such an operator is not included in the coroutines lib, I believe a lot of projects will add their own and we'll end up with no common name for this operation, which would be sad for the ecosystem IMO.

elizarov commented 2 years ago

Let's dissect it. I like the idea to use the complete verb to focus attention on the key purpose of this function. However, the After suffix might be somewhat confusing, since all the other xxxAfter functions (substringAfter and replaceAfter) take the element value as a parameter as opposed to the predicate. It is not a showstopper, since that xxxAfter suffix is used only in the strings and has not been used in collections, but still an annoying inconsistency. From the standpoint of the English language, After looks like the best choice, indeed. I cannot find any better way to express this intent.

However, Kotlin stdlib naming does not always clearly follow the predicate/element distinction and sometimes the same naming pattern is overloaded for both. There is a precedent that might yield a name that is more aligned with the rest of the Kotlin collections. Notice that there is indexOf(element), but indexOfFirst(predicate) and indexOfLast(predicate). Even for strings, xxxFirst and xxxLast functions always take a predicate as a parameter.

Following this precedent, I can suggest to consider an option of using a longer, but more explicit name for this function: completeAfterFirst

joffrey-bion commented 2 years ago

@elizarov thanks for the prompt consideration.

This is a very fair point, I hadn't payed enough attention to the "element vs predicate" distinction here. We could even consider having completeAfter(element) and completeAfterFirst(predicate), so we can really complete on a sentinel value too. Although as you mentioned in the transformWhile issue, you probably want to avoid having too many operators.

The downside of completeAfter[First] is that it cannot be backported to collections, as opposed to dropAfter[First]. But that might not be too much of a problem, given the main use case motivating this operator is making flows finite, and that would work at least for sequences as well.

dkhalanskyjb commented 8 months ago

Let's look at how transformWhile is used today (https://grep.app/search?q=transformWhile%20%7B).

Here are the usages where transformWhile is used exactly for completeAfter and the predicate is negated:

Here are the usages where transformWhile is used for completeAfter and the predicate is stated positively:

The examples where transformWhile is used for some other purpose:

These stats (although for a very small n) suggest that introducing a shorthand to transformWhile { emit(it); !predicate(it) } and naming it properly would reduce the number of boolean negations in the user code, as well as the number of errors: some people did name their copies takeUntil or something else that suggests that the element on which everything stops would not be included, making the name not quite right.