-
Notifications
You must be signed in to change notification settings - Fork 207
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
Conversation
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.
There was a problem hiding this 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/pubsub_v1/publisher/_sequencer/ordered_sequencer.py
Outdated
Show resolved
Hide resolved
@@ -25,6 +25,9 @@ | |||
_LOGGER = logging.getLogger(__name__) | |||
|
|||
|
|||
MessageType = Type[types.PubsubMessage] # type: ignore # pytype: disable=module-attr |
There was a problem hiding this comment.
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[]
.
There was a problem hiding this comment.
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)
A few trivial errors, will fix the checks tomorrow. |
@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 |
Towards #536.
This is a draft PR. The new noxfile check, mypy, passes locally, but there are still a few things to consider:
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 addsMarker file removed for the time being.py.typed
marker file, but we might want to postpone this addition until the generated code is error-free._TimeoutType
are defined in multiple places. We should probably extract the definition to a single central place within the library.PR checklist: