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

test(dtslint): add dtslint test for scan operator (#4093) #4347

Closed
wants to merge 2 commits into from

Conversation

dkosasih
Copy link
Contributor

Description: Add dtslint test for Scan operator

Related issue (if exists): #4093

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7645

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.806%

Totals Coverage Status
Change from base Build 7636: 0.0%
Covered Lines: 5244
Relevant Lines: 5417

💛 - Coveralls

});

it('should infer correctly with Array of T value', () => {
const a = of(1, 2, 3).pipe(scan((acc: number[], val: number, i) => [...acc, val])); // $ExpectType Observable<number[]>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without a seed of [] this should effect an error. It's a flaw in the typings. I'd add the []. I'll look at fixing the scan and reduce typings later; it's on my TODO list.

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'll be good if you could tag me when you do a PR for this. I'd like to learn how to resolve this kind of problem. Thanks Nick!

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

The typings for scan and reduce have issues. These will be addressed at a later date.

The simplest thing to do with these tests would be to replace them with the tests that were written for reduce, but change the operators and change the type expectations to be arrays instead of the the reduced values - e.g. Observable<number[]> instead of Observable<number>, etc.

More comprehensive tests can be written when the issues with the typings are addressed.

});

it('should infer correctly with type change accumulator', () => {
const a = of(1, 2, 3).pipe(scan((acc: string, val: number, i) => `${acc} ' ' ${val}`)); // $ $expectType Observable<string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

$ $expectType is a typo.

});

it('should infer correctly for accumulator of type array', () => {
const a = of(1, 2, 3).pipe(scan((x: number[], y: number, i: number) => x, [])); // $ExpectType Observable<number[]>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @cartant ,
I have changed the test to identically like reduce. But, I'm not sure about the type expectations you mentioned on the comment above - look like the typings are exactly the same as reduce.
Is this correct?

@benlesh
Copy link
Member

benlesh commented Jun 12, 2019

Sorry, @dkosasih, I'm going to close this one in favor of #4858. It seems that this one got a little behind, there was some competing work, etc... I'm sorry about that.

@benlesh benlesh closed this Jun 12, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants