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

Add type hints #64

Closed
wants to merge 7 commits into from
Closed

Add type hints #64

wants to merge 7 commits into from

Conversation

c-ryan747
Copy link

Hi,

I've just started to use grpclib and I noticed in #40 that you asked for help in typing the library, so here you go!

I've added type hints to most of the internals of grpclib as well as the streams that are exposed to users.

Due to the relative new-ness of type annotation in python I've had to bump the supported version of Python to 3.5.2, but as tox shows this PR passes all tests and lints fine. Mypy is also fine in non strict mode.

With this I've typed the examples, and introducing an error such as

        request = none_throws(await stream.recv_message())
        await stream.send_message(
            HelloReply(message='Hello, {}!'.format(request.nameWRONG)))

will throws a mypy error like

example/streaming/server.py:29: error: "HelloRequest" has no attribute "name_WRONG"

The main areas where I haven't introduced types are around the untyped h2 library and in areas where I'm unsure of the exact type (e.g. Metadata).

Let me know if I've made any of them too restrictive or if you have any other comments and I'll try to sort these out!

@vmagamedov
Copy link
Owner

Thank you for this effort! Would you mind to split this huge PR into several PRs to gradually cover codebase with type hints?

I was thinking to adopt mypy with two phases:

  1. cover most critical user-facing API (client.Stream, server.Stream, protoc plugin/stubs, examples), this will make user's codebase better
  2. cover grpclib internals, this will make grpclib better

Second phase can be broken into several steps:

  • independent modules like utils.py, protocol.py, metadata.py, stream.py, ...
  • server-side: server.py
  • client-side: client.py

Other things to consider:

  • starting from 0.3 release I plan to drop Python 3.5 support, since PyPy 7.0 gained Python 3.6 support, this might help
  • I have the events2 branch with a complex events system implemented, it would be hard to merge this branch into master after so many changes

I suggest to complete the first phase with minimal changes, and continue to gradually add mypy support after events2 branch is merged into master. The only reason this branch is not merged yet is that I wanted to fix all known bugs in 0.2.x release with Python 3.5 support. But it would be possible to backport these fixes from 0.3.x to 0.2.x if needed.

@c-ryan747
Copy link
Author

Yeah that makes sense, I've got some time on flights coming up this week so I'll split out the 'first stage' from this PR then

@vmagamedov
Copy link
Owner

Merged all the pending stuff into master, so all your upcoming work can be merged easily. And now you can use all the Python 3.6 features.

BTW, I think that it is too early to introduce none_throws utility function. I think that currently it is better to explicitly insert assert statements into code. Maybe in the future we will be able to overcome this issue in a better way, maybe by changing/extending recv_message method.

vmagamedov added a commit that referenced this pull request May 21, 2019
@c-ryan747 c-ryan747 closed this May 21, 2019
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.

2 participants