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

Port TaskSeq.takeWhileInclusive #122

Closed
bartelink opened this issue Dec 7, 2022 · 9 comments · Fixed by #126
Closed

Port TaskSeq.takeWhileInclusive #122

bartelink opened this issue Dec 7, 2022 · 9 comments · Fixed by #126
Labels
feature request New feature or enhancement request topic: surface area Adds functions to the public surface area

Comments

@bartelink
Copy link
Member

bartelink commented Dec 7, 2022

I'd like to see takeWhileInclusive (it has uses in Equinox) be ported from AsyncSeq.

Will post a PR to add it to the TODO list (and then one to implement it) in due course unless someone beats me to it, or there are objections to adding it...

@bartelink
Copy link
Member Author

bartelink commented Dec 8, 2022

Spike impl:

let TaskSeq_takeWhileInclusive predicate (source: taskSeq<_>) = taskSeq {
    use e = source.GetAsyncEnumerator(CancellationToken())
    let! step = e.MoveNextAsync()
    let mutable go = step
    while go do
        let value = e.Current
        yield value
        if predicate value then
            let! more = e.MoveNextAsync()
            go <- more
        else
            go <- false
}

(derived from Internal.tryPick + base impl )

In context: https://github.com/jet/equinox/blob/master/src/Equinox.Core/Internal.fs#L15-L45

@abelbraaksma
Copy link
Member

abelbraaksma commented Dec 10, 2022

Thanks for your suggestion @bartelink! So this is like “get next, yield that, test predicate on that one, if true, repeat”. That is, return element first, before testing predicate.

Interesting function. I was about to add a whole section (just like I did for seq) listing all functions that I’ll implement and which (likely) not.

Your proposal looks good, as does the code, feel free to issue a PR, it’ll need to have matching tests (I’m a little more rigorous than the AsyncSeq team in that regard and need notable corner cases), but I can help you with that.

@abelbraaksma
Copy link
Member

Btw, it should probably be added together with takeWhile and both should be paired with an async version for the predicate function. I’m happy to help with that.

@abelbraaksma abelbraaksma added topic: surface area Adds functions to the public surface area feature request New feature or enhancement request labels Dec 10, 2022
@bartelink
Copy link
Member Author

Thanks! Yes, I had noted that there was going to be no skimping on test coverage based on poking around ;)

But that's also why I confidently spent time converting Equinox and Propulsion over.

Won't get to the PR immediately but I'll get to it...

@abelbraaksma
Copy link
Member

Does it make sense to also add a skipWhileInclusive(Async)? In a separate PR, of course. What do you think? It is not in AsyncSeq.

@bartelink
Copy link
Member Author

bartelink commented Dec 14, 2022

That does seem to make sense (although I'm struggling to think of a time I've ever had the use case), but arguably you'd call it skipUntil ?
Of course, that would call the name of takeWhileInclusive into question, but it makes less sense there.
On balance the Inclusive-based name both on the skip and the take sides is probably best overall considering someone scanning intellisense to find the right tool for a job?

@abelbraaksma
Copy link
Member

but arguably you'd call it skipUntil ?

Seq has Seq.skipWhile and I like to stick with its naming conventions. They are each others opposites w.r.t. the predicate, so in the end I guess it is a matter of taste ;). See #130.

@abelbraaksma
Copy link
Member

This issue was implemented in #126 and will be published in the next version of NuGet (be it 0.4.0 or 0.3.x). 💯 @bartelink

@bartelink
Copy link
Member Author

Seq has Seq.skipWhile and I like to stick with its naming conventions

On reflection, my suggestion doesn't make as much sense as it did in my head as I typed it. I was suggesting to replace the usage of the term WhileInclusive with Until, on the basis that it would naturally imply the Inclusive aspect. But I wasn't considering the fact that Until does mean inverting the predicate. Either way I was definitely not suggesting not providing a takeWhile, or having two functions that only differ by the fact that the predicate's meaning is inverted (I suffer enough mental anguish there being a filter and a where as it is!)

130 would be the better for removing any reference to Until (but commenting here to keep the noise down!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or enhancement request topic: surface area Adds functions to the public surface area
Projects
None yet
2 participants