Skip to content

fix: improve typing of flatten to infer return iterator type #22

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

Open
forivall wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: improve typing of flatten to infer return iterator type #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/iterate.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,18 @@ describe('IteratorWithOperators', () => {
assert.equal(iterator.next().done, true)
})
})
describe('flatten', () => {
Copy link
Owner

@felixfbecker felixfbecker Oct 12, 2018

Choose a reason for hiding this comment

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

This test seems to be the exact same as in flatten.test.ts, so I don't understand the purpose? All the other tests in this file do not have tests in external files because they are implemented on the instance itself instead of external classes

Copy link
Contributor Author

@forivall forivall Oct 13, 2018

Choose a reason for hiding this comment

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

The main point for myself was to make sure that the type inference matches appropriately. Otherwise, it's just a smoke test that improves coverage. Perhaps it would be best to define the tests in a way to use them in both calling contexts.

Copy link
Owner

@felixfbecker felixfbecker Oct 13, 2018

Choose a reason for hiding this comment

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

Got it. Let's leave this out for now to not duplicate code. Type inference should be tested with https://github.com/Microsoft/dtslint but I don't have that set up yet.
In the future I would see this library use functions like RxJS instead of prototype methods so that would eliminate the coverage concerns.

it('should emit values from nested iterables', () => {
const collection = [1, [2, [3]], 4, [5]][Symbol.iterator]()
const iterator = new IteratorWithOperators(collection).flatten()
assert.equal(iterator.next().value, 1)
assert.equal(iterator.next().value, 2)
assert.deepEqual(iterator.next().value, [3])
assert.equal(iterator.next().value, 4)
assert.equal(iterator.next().value, 5)
assert.equal(iterator.next().done, true)
})
})
describe('reduce', () => {
it('should reduce all emitted values by calling the reducer', () => {
const iterator = new IteratorWithOperators([1, 2, 4, 3][Symbol.iterator]())
Expand Down
2 changes: 1 addition & 1 deletion src/iterate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export class IteratorWithOperators<T> implements IterableIterator<T> {
/**
* Returns a new Iterator that flattens items emitted by the Iterator a single level deep
*/
flatten<R>(): IteratorWithOperators<R> {
flatten<R>(this: IteratorWithOperators<R | Iterator<R> | Iterable<R>>): IteratorWithOperators<R> {
return new IteratorWithOperators(new FlattenIterator<R>(this.source))
}

Expand Down