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 Iterator#keepLast() to allow storing last iterator value #340

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lundibundi
Copy link
Member

  • code is properly formatted (npm run fmt)
  • tests are added/updated
  • documentation is updated (npm run doc to regenerate documentation based on comments)
  • description of changes is added under the Unreleased header in CHANGELOG.md

@lundibundi lundibundi requested a review from belochub July 2, 2020 09:11
lib/iterator.js Outdated
this.lastElement = undefined;
}

get last() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
get last() {
last() {

Copy link
Member

@nechaido nechaido left a comment

Choose a reason for hiding this comment

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

I do not like the proposed functionality as it propagates a programmer to save the iterator before the transformation. Leading to a situation where you have multiple iterators sharing a common state.

I would prefer to have a splitOn function, that will split iterator into two.

iter([1, 2, 3, 4, 5]).splitOn(v => v >= 3); // [iter([1, 2]), iter([3, 4, 5])]

constructor(base) {
super(base);
this.repeat = false;
this.lastElement = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like lastElement sound like instead of { value: T, done: boolean } it is T.

@lundibundi
Copy link
Member Author

I think the proposed splitOn is something like #339.

Will think about a better solution.

Leading to a situation where you have multiple iterators sharing a common state.

Though, what's wrong with this? I think it's already possible if someone wants to take values from the same iterator in multiple places (which is fine IMO) or misuses iterator (like using async function in .map etc).

@lundibundi lundibundi marked this pull request as draft July 6, 2020 11:28
@nechaido
Copy link
Member

nechaido commented Jul 6, 2020

Though, what's wrong with this? I think it's already possible if someone wants to take values from the same iterator in multiple places (which is fine IMO) or misuses iterator (like using async function in .map etc).

The problem is that it forces programmers to save iterator pre-transformation.
I know that it is currently possible to misuse iterators, but I believe that we should not encourage such patterns.

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.

2 participants