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

chore(distinctUntilKeyChanged): nail down key's typing with keyof T #3988

Merged

Conversation

imcotton
Copy link
Contributor

@imcotton imcotton commented Aug 3, 2018

Description:

With better typing constraint on T[key] looks up from TypeScript.

@imcotton imcotton changed the title chore(distinctUntilKeyChanged): nail down key's typing with keyof T [WIP] chore(distinctUntilKeyChanged): nail down key's typing with keyof T Aug 3, 2018
@imcotton imcotton force-pushed the nail-down-distinctUntilKeyChanged-key-type branch from 8b997d5 to 0a0c9f4 Compare August 3, 2018 20:09
@ci-reporter
Copy link

ci-reporter bot commented Aug 3, 2018

The build is failing

✨ Good work on this PR so far! ✨ Unfortunately, the Travis CI build is failing as of 0a0c9f4. Here's the output:

npm test
> @reactivex/rxjs@6.2.2 test /home/travis/build/ReactiveX/rxjs
> cross-env TS_NODE_PROJECT=spec/tsconfig.json mocha --opts spec/support/default.opts "spec/**/*-spec.ts"


/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:250
    return new TSError(diagnosticText, diagnosticCodes)
           ^
TSError: ⨯ Unable to compile TypeScript:
spec/operators/distinctUntilKeyChanged-spec.ts(209,68): error TS2345: Argument of type '(x: number, y: number) => boolean' is not assignable to parameter of type '<K>(x: K, y: K) => boolean'.
  Types of parameters 'x' and 'x' are incompatible.
    Type 'K' is not assignable to type 'number'.
spec/operators/distinctUntilKeyChanged-spec.ts(225,68): error TS2345: Argument of type '(x: number, y: number) => boolean' is not assignable to parameter of type '<K>(x: K, y: K) => boolean'.
  Types of parameters 'x' and 'x' are incompatible.
    Type 'K' is not assignable to type 'number'.

    at createTSError (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:250:12)
    at getOutput (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:358:40)
    at Object.compile (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:545:11)
    at Module.m._compile (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:430:43)
    at Module._extensions..js (module.js:663:10)
    at Object.require.extensions.(anonymous function) [as .ts] (/home/travis/build/ReactiveX/rxjs/node_modules/ts-node/src/index.ts:433:12)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at /home/travis/build/ReactiveX/rxjs/node_modules/mocha/lib/mocha.js:231:27
    at Array.forEach (<anonymous>)
    at Mocha.loadFiles (/home/travis/build/ReactiveX/rxjs/node_modules/mocha/lib/mocha.js:228:14)
    at Mocha.run (/home/travis/build/ReactiveX/rxjs/node_modules/mocha/lib/mocha.js:536:10)
    at Object.<anonymous> (/home/travis/build/ReactiveX/rxjs/node_modules/mocha/bin/_mocha:582:18)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:191:16)
    at bootstrap_node.js:612:3

I'm sure you can fix it! If you need help, don't hesitate to ask a maintainer of the project!


This comment was automagically generated by ci-reporter. If you see a problem, open an issue here.

@imcotton imcotton force-pushed the nail-down-distinctUntilKeyChanged-key-type branch from 0a0c9f4 to 9b4d649 Compare August 3, 2018 23:21
@coveralls
Copy link

coveralls commented Aug 3, 2018

Coverage Status

Coverage remained the same at 96.951% when pulling bc9768d on imcotton:nail-down-distinctUntilKeyChanged-key-type into bb77e25 on ReactiveX:master.

@imcotton imcotton changed the title [WIP] chore(distinctUntilKeyChanged): nail down key's typing with keyof T chore(distinctUntilKeyChanged): nail down key's typing with keyof T Aug 3, 2018
@imcotton imcotton force-pushed the nail-down-distinctUntilKeyChanged-key-type branch from 9b4d649 to cd0dacd Compare August 4, 2018 00:42
@imcotton imcotton force-pushed the nail-down-distinctUntilKeyChanged-key-type branch 2 times, most recently from cce8f56 to 678d0a5 Compare August 4, 2018 01:47
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.

Thanks for the PR.

LGTM, but could you add a couple of dtslint-based tests?

I'd like all typings changes to be accompanied by dtslint-based tests - so that we can build out the coverage.

In particular, I'd like to see a test that expects an error if the K is not a key of T - to guard against a regression.

For an example, have a look at https://github.com/ReactiveX/rxjs/blob/master/spec-dtslint/operators/concatAll-spec.ts

To run the dtslint tests, use this command:

npm run dtslint

@imcotton imcotton force-pushed the nail-down-distinctUntilKeyChanged-key-type branch from 678d0a5 to 05e71a3 Compare August 5, 2018 09:08
@imcotton imcotton force-pushed the nail-down-distinctUntilKeyChanged-key-type branch from 05e71a3 to bc9768d Compare August 5, 2018 09:11
@imcotton
Copy link
Contributor Author

imcotton commented Aug 5, 2018

Hi, thanks for the guidance of dtslint, I've add a couple of cases into its dtslint spec, hopefully it covers everything, feel free to pointing out if need further improvements.

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.

LGTM

@benlesh benlesh merged commit 4ec4ff1 into ReactiveX:master Aug 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Sep 5, 2018
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.

4 participants