Repository navigation
feat(mergeScan): Add index to the accumulator function - #4458
Conversation
Pull Request Test Coverage Report for Build 8001
💛 - Coveralls |
cartant
left a comment
There was a problem hiding this comment.
LGTM, but would you be able to re-write the test to use marble diagram?
|
@cartant I think the test is fine given it's just asserting the index is being passed to the function. |
|
|
||
| it('should support an index parameter', () => { | ||
| const o = of(1, 2, 3).pipe(mergeScan((acc, value, index) => of(index), 0)); // $ExpectType Observable<number> | ||
| }); |
There was a problem hiding this comment.
this new test is unnecessary. :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| return of(x); | ||
| }, 0)).subscribe(); | ||
|
|
||
| expect(recorded).to.deep.equal(expected); |
There was a problem hiding this comment.
Can you please move expected inside of equal here? It will make the test moderately more readable, IMO.
705e8cb to
dfd6213
Compare
Generated by 🚫 dangerJS |
|
Thank you, @martinsik! |
Description:
This PR adds
indexparameter to the accumulator function used bymergeScanjust like in most other operators.Related issue (if exists):
Closes #4441