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

process: make sure type annoations pass with mypy #542

Merged
merged 21 commits into from Nov 30, 2021

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Nov 23, 2021

Towards #536.

This is a draft PR. The new noxfile check, mypy, passes locally, but there are still a few things to consider:

  • The generated code is omitted from type checks, because mypy reports several errors for it
    (will be done separately, since it's currently blocked by Generated code for Pub/Sub does not pass type checks with mypy gapic-generator-python#1092 )
  • The PR adds py.typed marker file, but we might want to postpone this addition until the generated code is error-free. Marker file removed for the time being.
  • Some type aliases such as _TimeoutType are defined in multiple places. We should probably extract the definition to a single central place within the library.

PR checklist:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 23, 2021
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the googleapis/python-pubsub API. label Nov 23, 2021
google-api-core versions prior to v2.2.2 lack the definition of
_MethodDefault, thus a workaround is needed for that.
The autogenerated code does not pass mypy type checks yet, thus
we should not advertise the package as type-checked.
@plamut plamut marked this pull request as ready for review November 24, 2021 20:48
@plamut plamut requested review from a team as code owners November 24, 2021 20:48
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

Overall, I'd like to see us using only one typing checker, and I think 'mytype' is the consensus favorite.

google/cloud/__init__.py Outdated Show resolved Hide resolved
google/cloud/pubsub_v1/publisher/client.py Outdated Show resolved Hide resolved
@@ -25,6 +25,9 @@
_LOGGER = logging.getLogger(__name__)


MessageType = Type[types.PubsubMessage] # type: ignore # pytype: disable=module-attr
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the type: ignore here, and I think this may be a misuse of Type[].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ignore here is needed, because PubsubMessage is dynamically injected into google/cloud/pubsub_v1/types.py and both type checkers think it does not exist. The alias was created to not repeat the same ignore comment in every line where message is a method parameter.

Using a plain types.PubsubMessage in the alias results in mypy complaining that variable MessageType is not valid as a type. Using Type[...] works around that.

(alternatives welcome, especially if they are canonical)

google/cloud/pubsub_v1/subscriber/_protocol/dispatcher.py Outdated Show resolved Hide resolved
google/cloud/pubsub_v1/subscriber/_protocol/leaser.py Outdated Show resolved Hide resolved
@plamut plamut requested a review from tseaver November 26, 2021 21:59
@plamut
Copy link
Contributor Author

plamut commented Nov 26, 2021

A few trivial errors, will fix the checks tomorrow.

@plamut plamut requested a review from tseaver November 27, 2021 09:00
@plamut
Copy link
Contributor Author

plamut commented Nov 27, 2021

@tseaver The required changes turned out to be more than just a few trivial lines, thus please take another look when you manage.

It's mostly just getting rid of cast() for Optional types, and one extra case in Dispatcher had to be covered after splitting batch_commands into separate lists (coverage complaint).

@plamut plamut merged commit 8f32dd4 into googleapis:main Nov 30, 2021
@plamut plamut deleted the iss-536 branch November 30, 2021 14:59
@plamut plamut restored the iss-536 branch December 17, 2021 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/python-pubsub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants