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

Fix type hints for pipe using overloads. Fixes #355, #285 #372

Merged
merged 7 commits into from
May 11, 2019

Conversation

dbrattli
Copy link
Collaborator

@dbrattli dbrattli commented May 10, 2019

This PR fixes the typing issues with the pipe operator using typing overloads. The downside is that pylint does not support this yet and only sees the first overload (pylint-dev/pylint#2239). The good thing is that PyCharm does the right thing and see all the overloads and correctly infers the last type.

Skjermbilde 2019-05-10 kl  17 40 30

Thus I would rather do it the right way since PyCharm already supports this, and let pylint problems be pylint problems. This is similary to how the typing were fixed with typescript in RxJS (https://github.com/ReactiveX/rxjs/blob/master/src/internal/Observable.ts). Hopefully pylint will eventually be fixed.

@dbrattli dbrattli changed the title Fix typing for pipe using overloads. Fixes #355. Fix type hints for pipe using overloads. Fixes #355. May 10, 2019
@dbrattli dbrattli changed the title Fix type hints for pipe using overloads. Fixes #355. Fix type hints for pipe using overloads. Fixes #355 and #285 May 10, 2019
@dbrattli dbrattli changed the title Fix type hints for pipe using overloads. Fixes #355 and #285 Fix type hints for pipe using overloads. Fixes #355, #285 May 10, 2019
@coveralls
Copy link

coveralls commented May 10, 2019

Coverage Status

Coverage increased (+0.06%) to 92.6% when pulling 56bc897 on feature/pipe-typing into 1de2850 on master.

@dbrattli
Copy link
Collaborator Author

I really don't like that pylint handles this code so badly. I will try to see if I can disable it and only use mypy or try some other linter. Here is another pylint issue that tracks this problem pylint-dev/pylint#1581

@dbrattli
Copy link
Collaborator Author

dbrattli commented May 11, 2019

The latest change with having an Observable specific pipe() overload first seems to make pylint happy, so now pylint, mypy and pyre are happy when you pipe your operators. Adding docstring to the first overload also make vscode happy when you inpect or auto-complete.

Copy link
Collaborator

@jcafhe jcafhe left a comment

Choose a reason for hiding this comment

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

That looks good to me 👍

With a simple example:

x = rx.just(1).pipe(
    ops.map(lambda i: i),
    ops.to_future(),
    ops.map(lambda i:i),
    )

I've got a nice error with mypy:
pipe_typ.py:4: error: Argument 2 to "pipe" of "Observable" has incompatible type "Callable[[Observable], Future[Any]]"; expected "Callable[[Observable], Observable]"

Just a side note, to check a script which imports rx with mypy, I had to hack the setup.py and create an empty file py.typed. Basically, this just follows the procedure detailled in making-pep-561-compatible-packages. I'm not sure if this is 100% required, or if it's just me?

@dbrattli
Copy link
Collaborator Author

We should probably add py.typed. Didn't know about it so thanks for the link.

I btw started using Pyre today. Very nice type checking and integration with vscode: https://pyre-check.org/

Copy link
Collaborator

@erikkemperman erikkemperman left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Coveralls reports slightly lower values, I think it is mostly due to the ellipses. Perhaps you can add something like the following to .coveragerc to make the build green.

...  # pylint: disable=pointless-statement

Or maybe without the comment. Approving regardless.

@dbrattli dbrattli merged commit 89df178 into master May 11, 2019
@dbrattli dbrattli deleted the feature/pipe-typing branch June 10, 2019 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants