-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add count method #6
Conversation
Sorry about not getting back to you sooner on this one. Nicely done with the code. I have a concern that has nothing to do with your code. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your code is fine, I am just worried about confusion generated because someone doesn't understand why the sequence was consumed.
it('test count with 2 elements', () => { | ||
const values: number[] = [18, 7]; | ||
expect(genSequence(values).count()).to.equal(2); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done. There is two more tests necessary.
Need to test the behavior of what happens if the sequence is used again after .count()
.
Example:
const values = [1, 2, 3, 4, 5, 6]
const seq = genSequence(values);
const cnt = seq.count();
const cntAgain = seq.count();
and
const values = [1, 2, 3, 4, 5, 6]
const seq = genSequence(values).filter(a => !!(a % 2));
const cnt = seq.count();
const cntAgain = seq.count();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.NET has the same issue with IEnumerables and LINQ, but they allow it, and many times the sequence can handle a reuse.
Sequence loses the "next" function because it had to reset when it saw done which felt a bit strange.
I made sequences reusable. I ended up removing the next() method because the iterator needed to be reset after it saw done. I thought maybe that was a bad practice, but the spec doesn't seem to prohibit it, so if there are good scenarios for next() we could bring it back for reusable sequences. This was kind of an experiment to see if I could make them reusable. It turned out well, so I thought I would run it by you. Let me know what you think. |
Wow. Very cool. I think your suggestion is worth exploring. I will have to bump the major version number, since it is a change to the underlying behavior. Typing and thinking at the same time (not a good combo for me):
|
1 similar comment
I added back |
I was really wishing I had count today, while using genSequence so I added it.