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
chore: ensure mypy passes for package #242
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.
Shouldn't we be adding a nox session for mypy?
google/cloud/datastore/client.py
Outdated
def no_datastore_api(self): | ||
return None | ||
|
||
make_datastore_api = no_datastore_api |
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.
Why isn't this just a function?
def no_datastore_api(self): | |
return None | |
make_datastore_api = no_datastore_api | |
def make_datastore_api(self): | |
return None |
And maybe it should raise an exception, since the only reason we're here is that gRPC is disabled?
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.
this would be a lambda but flake8 isn't a fan and I am not going to protest loudly about it. This is a minimal change that ensures make_datastore_api is consistently a callable.
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 just don't see the point in defining the no_datastore_api
function, only to alias it as make_datastore_api
-- just name the function make_datastore_api
.
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 get it, but if you do that you end up fighting with google/cloud/datastore/client.py:41: error: All conditional function variants must have identical signatures
:\ This seems not terrible?
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 pushed d6b600a, which passes mypy, and removes the extra no_database_api
function.
This can merge, but bug should have a comment added that we want to evaluate mypy against This PR needs to merge to remove a class of errors from generated code https://www.github.com/googleapis/gapic-generator-python/issues/1026 |
Also, have shim 'make_datastore_api' raise an exception, as it only gets called if gRPC is disabled, or not installed.
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
related #240