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#zipLongest and improve zip description #349

Open
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

Refs: https://docs.python.org/3.0/library/itertools.html#itertools.zip_longest

lib/iterator.js Outdated
// Returns an iterator of Arrays where the i-th tuple contains
// the i-th element from each of the passed iterators.
// The iterator stops when the shortest input iterable is exhausted.
// Signature: base, ...iterators
Copy link
Member

Choose a reason for hiding this comment

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

It was written this way to specifically hide the implementation detail of having a separate argument which has the same meaning as every other

Suggested change
// Signature: base, ...iterators
// Signature: ...iterators

lib/iterator.js Outdated
// If the iterables are of uneven length, missing values are filled-in
// with <undefined>. Iteration continues until the longest iterable
// is exhausted.
// Signature: base, ...iterators
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Signature: base, ...iterators
// Signature: ...iterators

lib/iterator.js Outdated
// Returns an iterator of Arrays where the i-th tuple contains
// the i-th element from each of the passed iterators.
// The iterator stops when the shortest input iterable is exhausted.
// Signature: ...iterators
Copy link
Member

Choose a reason for hiding this comment

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

No need for the signature change when the default is correct

Suggested change
// Signature: ...iterators

Copy link
Member Author

Choose a reason for hiding this comment

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

The metadoc will generate zip(iterators)

lib/iterator.js Outdated
// If the iterables are of uneven length, missing values are filled-in
// with <undefined>. Iteration continues until the longest iterable
// is exhausted.
// Signature: ...iterators
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Suggested change
// Signature: ...iterators

// Signature: ...iterators
// iterators <Array>
// Returns: <Iterator>
zipLongest(...iterators) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should allow specifying the value that should be used to fill with when there are no values left in one of the iterators.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I couldn't devise a way to make a proper API.

Creates an iterator that aggregates elements from each of the iterators.
If the iterables are of uneven length, missing values are filled-in with
undefined or provided defaultValue. Iteration continues until the
longest iterable is exhausted.
@belochub belochub added the semver-minor addition of functionality in a backwards-compatible manner label Jul 31, 2020
Copy link
Member

@belochub belochub left a comment

Choose a reason for hiding this comment

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

LGTM with one nit

//
// Returns an iterator of Arrays where the i-th tuple contains
// the i-th element from each of the passed iterators.
// The iterator stops when the shortest input iterable is exhausted.
Copy link
Member

Choose a reason for hiding this comment

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

Here and in other places where you're mentioning the iterable protocol in combination with "exhausting", you should use the iterator contract instead since iterable only requires for an object to have a function that returns an iterator, while the iterator is an actual object that has a next() method and that can be "exhausted" by calling that method.

Suggested change
// The iterator stops when the shortest input iterable is exhausted.
// The iterator stops when the shortest input iterator is exhausted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor addition of functionality in a backwards-compatible manner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants