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

Implement TaskSeq.takeUntil, takeUntilAsync, takeUntilInclusive and takeUntilInclusiveAsync #183

Closed
wants to merge 2 commits into from

Conversation

abelbraaksma
Copy link
Member

@abelbraaksma abelbraaksma commented Oct 30, 2023

This partially fixes #130.

These are basically the counterparts to takeWhile. This implements:

  • takeUntil
  • takeUntilAsync
  • takeUntilInclusive
  • takeUntilInclusiveAsync

Copy link
Member

@bartelink bartelink left a comment

Choose a reason for hiding this comment

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

I'm bit confused here - there's a thread here: #122 (comment)

AIUI:

  • there's a takeWhileInclusive already
  • there's a takeUntil (and a takeWhileInclusive) in AsyncSeq - the only semantic difference is that the predicate is inverted in the until case
  • Seq has filter and where - same signatures and impl, but there's no where here - i.e. why the redundancy for this but not for that?

Given the whileInclusive has only appeared in 0.3.0 (and that has broken use/finally semantics), I'd be in favor of a 0.3.1 or 0.4.0 that picks Until or WhileInclusive and only provides exactly one of them. Ultimately while this is a plumbing library following a pattern and most things can be intuited, people will nonetheless ultimately will be confronted with a long functions list and/or intellisense picker, and sowing confusion while also making it longer outweighs the benefits of one or other name "speaking" to some given user instantly. I think Until would be my preference, but WhileInclusive has the benefit of being direct and unambiguous. Interesting to note that in rxJS, they already used Until for another mechanism, and opted to provide the Inclusive capability as an optional flag on takeWhile. I can be persuaded out of it, but it seems that holding off on using a new word might be best - XyzInclusive is pretty unambiguous, and Seq does not have an Until? I note also there might be a move to static type to enable leaning on overloading - in that case, removing N Take/WhileInclusive methods and the Until overloads would reduce the perceived size/complexity of the library? (Yes, some code might become a bit uglier, but IME Inclusive is a pretty rare thing to require?)

If both are to be retained, I wonder whether there's a way to do the tests to make it clear that one is the same as the other with a flipped condition? I know there's a fine line between replicating the impl in the test and missing something, but there's a lot of value in being able to cross check the validation for one with the other without leaning on painful and error prone textual diffs...

@abelbraaksma
Copy link
Member Author

abelbraaksma commented Oct 30, 2023

If both are to be retained, I wonder whether there's a way to do the tests to make it clear that one is the same as the other with a flipped condition?

Good point. I may consider updating this using such tests instead.

To the point of retaining or not. I have always missed a takeUntil in seq. The reason I also have the takeUntilInclusive is mainly because of your suggestion on takeWhile and posts like these: https://stackoverflow.com/questions/12562327/how-to-do-seq-takewhile-one-item-in-f.

But I like the suggestion of overloading instead. That's probably a better way to go.

@abelbraaksma
Copy link
Member Author

PS: all suggestions have been applied to #136, it was by accident that this PR included those changes as well. I will rebase.

@abelbraaksma abelbraaksma force-pushed the implement-takeUntil-and-variants branch from 17f3400 to cb6a852 Compare October 30, 2023 12:12
@abelbraaksma
Copy link
Member Author

abelbraaksma commented Oct 30, 2023

Ok, rebased this such that it is now properly based off of main instead. See for the doc-comment changes that I applied from your review comments: #136 (/cc @bartelink).

@bartelink
Copy link
Member

Apologies for not paying attention to your responses on my comments as I made them
Sounds like we are on the same page
The final one alluding to Async.Parallel is the crux of it all
I need to be afk for most of the rest of the day now (and am travelling later this week so will have simialrly patchy responsiveness) but should hopefully be able to fit in responding to your responses and re-reviewing later on

@abelbraaksma abelbraaksma force-pushed the implement-takeUntil-and-variants branch from cb6a852 to 9dccfc1 Compare October 30, 2023 12:19
@abelbraaksma
Copy link
Member Author

Awesome! Much appreciated! :)

@abelbraaksma abelbraaksma force-pushed the implement-takeUntil-and-variants branch from 9dccfc1 to 18cbcbc Compare October 31, 2023 11:18
@abelbraaksma abelbraaksma added topic: surface area Adds functions to the public surface area feature request New feature or enhancement request labels Oct 31, 2023
@abelbraaksma abelbraaksma force-pushed the implement-takeUntil-and-variants branch from 18cbcbc to 405daea Compare October 31, 2023 13:31
This adds `TaskSeq.takeUntil`, `TaskSeq.takeUntilAsync`, `TaskSeq.takeUntilInclusive` and `TaskSeq.takeUntilInclusiveAsync`, plus tests.
@abelbraaksma abelbraaksma force-pushed the implement-takeUntil-and-variants branch from 405daea to bfb12ae Compare October 31, 2023 13:35
@abelbraaksma
Copy link
Member Author

I'm gonna kill this one off. The main use-case would be that it isn't trivial to write takeUntil through takeWhile, and this would help users write clearer code. Another use-case would be that if takeUntil is needed and you would write takeWhile (f >> not) this would not perform as well, however that issue is not resolved here.

We can always revisit once we have sorted out the other low-hanging fruit.

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
Development

Successfully merging this pull request may close these issues.

Add skipWhile, skipWhileInclusive with async variants, and takeUntil etc
2 participants