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.

Sign up for GitHub

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

Jump to bottom

take_until and map_until #597

Closed
JohnScience opened this issue Jan 15, 2022 · 3 comments · Fixed by #616
Closed

take_until and map_until #597

JohnScience opened this issue Jan 15, 2022 · 3 comments · Fixed by #616

Comments

@JohnScience
Copy link

JohnScience commented Jan 15, 2022 •
edited
Loading

A while ago there was a proposal by Jon Gjengset (jonhoo) to add take_until into Rust's standard library:
rust-lang/rust#62208

While it may be not a good fit for the standard library, it might be a worthwhile addition to itertools.

As michaelsproul pointed out in the discussion,

I don't think take_while_ref or peeking_take_while are well suited to this because they require you to assign the iterator to a local variable, and muck around with mutation, which breaks the flow of the iterator chain. The best I could do with them was this, which isn't as nice as take_until.

If you don't mind, I'd be happy to try and get your code into itertools, giving you credit of course! (Co-authored-by on the commit)
@JohnScience JohnScience mentioned this issue Jan 16, 2022
Open
@Samyak2
Copy link

Samyak2 commented Feb 21, 2022

I would like to take this up.

I needed this functionality for a project but noticed that this is neither present in the standard library nor itertools.

Samyak2 added a commit to psiayn/spressolisp that referenced this issue Feb 21, 2022
@Samyak2
feat: nicer errors for lambda calls and fix error prop
a2a4523 
Lambda now stores tokens of the params:
- Beacuse we store the params as just a vec of strings, we can't store
  tokens inside each param.
- Impl TokenHoarder and TokenGiver for Lambda, but these must only be
  used for tokens of the parameters, not the body
- Impl constructor for Lambda since the `param_tokens` are private

Better error display in lambda:
- During creation, lambdas now store the tokens of the parameters
- Shows nice error if wrong number of args are given to `lambda` the
  built-in function
- Shows nice error if any of the params are not a symbol
- Shows nice error if number of args and params mismatch - shows both

Fix lambda return value bug:
- When the body of a lambda had an error, it would return `false`
  instead of showing the error
- This was because `take_while` would consume the `Result` which had
  error, discard it and stop taking.
    - when it was the only expr in the body, it would return an empty
      iterator which causes the default value in `unwrap_or` to be
      returned (which was `false`)
- Replaced the take_while with a manual loop
    - returns the first error we see and does not consume further
    - keeps storing the last (not error) result
    - returns the last result or `false` is nothing is available
- Note: this could have been written a little more elegantly using
  something like `take_until`:
    - rust-lang/rust#62208
    - rust-itertools/itertools#597
@junbl junbl mentioned this issue May 23, 2022
Merged
@junbl
Copy link

junbl commented May 23, 2022

I'm curious how y'all envision a map_until function would work.

The difference between take_while and take_until being that we keep the first element for which the predicate isn't satisfied, what would we do with that first non-satisfying element in the map version? I'm assuming that the setup is the same as map_while and we're passing a function (I::Item) -> Option<T>. We can't return it unchanged, since it'll likely be of the wrong type, and we don't have a T since it returned None.

Maybe a map_while_ref would fit that use case better?

@junbl
Copy link

junbl commented May 25, 2022

I've been thinking about it, and I think the take_until name is confusing, as it implies a reversal of the predicate (take until this is satisfied vs. take while this is satisfied) which is unrelated to the actual functionality. take_while_inclusive more clearly shows the relationship with the existing method and the purpose of this adaptor.

@junbl junbl mentioned this issue Nov 29, 2022
Closed
@bors bors bot closed this as completed in c5b64c9 Jun 19, 2023
@OlegAlexander OlegAlexander mentioned this issue Jul 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add take_while_inclusive method junbl/itertools
3 participants
@JohnScience @junbl @Samyak2