-
Notifications
You must be signed in to change notification settings - Fork 190
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
refactor: python typing improvements #2537
Conversation
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.
Awesome code quality improvement, thank you!
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.
LGTM, a few very small things
@@ -759,7 +765,7 @@ def to_response(b): | |||
else: | |||
context.service_context.abort(grpc.StatusCode.INVALID_ARGUMENT, | |||
'Invalid query.') | |||
return None | |||
raise ValueError |
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.
nit: Either pass a somewhat descriptive string to this ValueError, or, since it should be unreachable, remove it entirely.
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.
Added a comment to say that this is unreachable, but is there for the type checker to understand that code does not flow past this point.
A series of typing improvements I made while working on the cursor changes. No logic or behavior changes have been made, so should perform identically with the existing deployed code.
Changes:
--mypy_out=
flag for protoc. This generates a typing stub for the grpc protobuf output, which can be used by different python type checking tools/extensions. (does not have to be mypy)models.py
Sadly yield still causes problems typing wise, so turning on "strict" typing is still not possible, details for anyone interested in why: