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

chore: ensure mypy passes for package #242

Merged
merged 9 commits into from
Oct 14, 2021
Merged

chore: ensure mypy passes for package #242

merged 9 commits into from
Oct 14, 2021

Conversation

crwilcox
Copy link
Contributor

@crwilcox crwilcox commented Oct 13, 2021

related #240

mypy google/cloud/datastore
Success: no issues found in 12 source files
mypy -p google.cloud.datastore --no-incremental
Success: no issues found in 12 source files

@crwilcox crwilcox requested a review from a team as a code owner October 13, 2021 17:09
@product-auto-label product-auto-label bot added the api: datastore Issues related to the googleapis/python-datastore API. label Oct 13, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 13, 2021
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.

Shouldn't we be adding a nox session for mypy?

Comment on lines 39 to 42
def no_datastore_api(self):
return None

make_datastore_api = no_datastore_api
Copy link
Contributor

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?

Suggested change
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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

@crwilcox crwilcox added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 14, 2021
@crwilcox
Copy link
Contributor Author

This can merge, but bug should have a comment added that we want to evaluate mypy against google and tests (comment in code)

This PR needs to merge to remove a class of errors from generated code https://www.github.com/googleapis/gapic-generator-python/issues/1026

@crwilcox crwilcox removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 14, 2021
Also, have shim 'make_datastore_api' raise an exception, as it only
gets called if gRPC is disabled, or not installed.
@google-cla
Copy link

google-cla bot commented Oct 14, 2021

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 @googlebot I consent. in this pull request.

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 cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Oct 14, 2021
@tseaver
Copy link
Contributor

tseaver commented Oct 14, 2021

@googlebot I consent.

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Oct 14, 2021
@crwilcox crwilcox merged commit 6398bbc into main Oct 14, 2021
@crwilcox crwilcox deleted the mypy branch October 14, 2021 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the googleapis/python-datastore 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