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

feat(mergeScan): Add index to the accumulator function #4458

Merged
merged 5 commits into from
Jan 30, 2019
Merged

feat(mergeScan): Add index to the accumulator function #4458

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ad6c91a
feat(mergeScan): add index to the accumulator function
martinsik Jan 8, 2019
f27b098
test(mergeScan): use marbles instead of "of()"
martinsik Jan 9, 2019
dfd6213
test(mergeScan): update spec, remove unnecessary dtest
martinsik Jan 9, 2019
385e04c
Merge branch 'master' into feat-4441-add-index-to-mergeScan
benlesh Jan 30, 2019
16bbed8
readd index lost in merge.
benlesh Jan 30, 2019
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
4 changes: 4 additions & 0 deletions spec-dtslint/operators/mergeScan-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ it('should support a currency', () => {
const o = of(1, 2, 3).pipe(mergeScan((acc, value) => of(acc + value), '', 47)); // $ExpectType Observable<string>
});

it('should support an index parameter', () => {
const o = of(1, 2, 3).pipe(mergeScan((acc, value, index) => of(index), 0)); // $ExpectType Observable<number>
});
Copy link
Member

Choose a reason for hiding this comment

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

this new test is unnecessary. :)

Copy link
Collaborator

@cartant cartant Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm pretty sure this test will fail if the index is removed from the signature, so it guards against a regression, IMO.

Nah. You're right. The spec will fail first. Duh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this test. I was looking at how is this thing tested elsewhere and it seemed to be the same usecase as here https://github.com/ReactiveX/rxjs/blob/master/spec-dtslint/operators/map-spec.ts#L12-L14 or here https://github.com/ReactiveX/rxjs/blob/master/spec-dtslint/operators/findIndex-spec.ts#L8-L10

Copy link
Collaborator

@cartant cartant Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Personally, I'd favour the dtslint typing tests being written without regard to what's tested in the specs - even if that means there are some redundant tests.


it('should enforce types', () => {
const o = of(1, 2, 3).pipe(mergeScan()); // $ExpectError
});
Expand Down
13 changes: 13 additions & 0 deletions spec/operators/mergeScan-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,4 +419,17 @@ describe('mergeScan', () => {
expectSubscriptions(inner[1].subscriptions).toBe(ysubs);
expectSubscriptions(inner[2].subscriptions).toBe(zsubs);
});

it('should pass current index to accumulator', () => {
const recorded: number[] = [];
const expected = [0, 1, 2, 3];
const e1 = of('a', 'b', 'c', 'd');

e1.pipe(mergeScan((acc, x, index) => {
recorded.push(index);
return of(x);
}, 0)).subscribe();

expect(recorded).to.deep.equal(expected);
Copy link
Member

Choose a reason for hiding this comment

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

Can you please move expected inside of equal here? It will make the test moderately more readable, IMO.

});
});
8 changes: 4 additions & 4 deletions src/internal/operators/mergeScan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,14 @@ import { ObservableInput, OperatorFunction } from '../types';
* @method mergeScan
* @owner Observable
*/
export function mergeScan<T, R>(accumulator: (acc: R, value: T) => ObservableInput<R>,
export function mergeScan<T, R>(accumulator: (acc: R, value: T, index: number) => ObservableInput<R>,
seed: R,
concurrent: number = Number.POSITIVE_INFINITY): OperatorFunction<T, R> {
return (source: Observable<T>) => source.lift(new MergeScanOperator(accumulator, seed, concurrent));
}

export class MergeScanOperator<T, R> implements Operator<T, R> {
constructor(private accumulator: (acc: R, value: T) => ObservableInput<R>,
constructor(private accumulator: (acc: R, value: T, index: number) => ObservableInput<R>,
private seed: R,
private concurrent: number) {
}
Expand All @@ -77,7 +77,7 @@ export class MergeScanSubscriber<T, R> extends OuterSubscriber<T, R> {
protected index: number = 0;

constructor(destination: Subscriber<R>,
private accumulator: (acc: R, value: T) => ObservableInput<R>,
private accumulator: (acc: R, value: T, index: number) => ObservableInput<R>,
private acc: R,
private concurrent: number) {
super(destination);
Expand All @@ -86,7 +86,7 @@ export class MergeScanSubscriber<T, R> extends OuterSubscriber<T, R> {
protected _next(value: any): void {
if (this.active < this.concurrent) {
const index = this.index++;
const ish = tryCatch(this.accumulator)(this.acc, value);
const ish = tryCatch(this.accumulator)(this.acc, value, index);
const destination = this.destination;
if (ish === errorObject) {
destination.error(errorObject.e);
Expand Down