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

feat: improve type information #176

Merged
merged 19 commits into from Oct 23, 2020

Conversation

HemangChothani
Copy link
Contributor

Fixes #139

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 27, 2020
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.

@HemangChothani Please rebase / retarget your changes against the current master branch. The v2-staging branch is no longer active (@crwilcox force-pushed it to master last week).

@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/python-firestore API. label Aug 28, 2020
@HemangChothani HemangChothani changed the base branch from v2-staging to master August 28, 2020 10:05
@tseaver tseaver dismissed their stale review September 1, 2020 21:06

PR retargeted to master.

@HemangChothani HemangChothani linked an issue Sep 7, 2020 that may be closed by this pull request
@tseaver tseaver added the status: blocked Resolving the issue is dependent on other work. label Sep 16, 2020
@tseaver
Copy link
Contributor

tseaver commented Sep 16, 2020

Blocked on #188.

@tseaver tseaver removed the status: blocked Resolving the issue is dependent on other work. label Sep 24, 2020
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.

@crwilcox Please check my comments here: my typing-fu may not be strong enough.

google/cloud/firestore_v1/_helpers.py Outdated Show resolved Hide resolved
google/cloud/firestore_v1/base_document.py Outdated Show resolved Hide resolved
google/cloud/firestore_v1/document.py Outdated Show resolved Hide resolved
google/cloud/firestore_v1/document.py Outdated Show resolved Hide resolved
google/cloud/firestore_v1/document.py Outdated Show resolved Hide resolved
google/cloud/firestore_v1/transaction.py Outdated Show resolved Hide resolved
google/cloud/firestore_v1/transaction.py Outdated Show resolved Hide resolved
@crwilcox
Copy link
Contributor

@tseaver I think your comments are generally correct. Not sure my stance on optional vs union with none, but they should be ~equivalent.

@HemangChothani HemangChothani requested a review from a team October 7, 2020 14:48
@HemangChothani
Copy link
Contributor Author

@tseaver PTAL

@crwilcox
Copy link
Contributor

crwilcox commented Oct 20, 2020

@tseaver I think your suggestions make sense. Unfortunately they are out of date so I cannot auto-apply

I think Union with None is clearer what is going on with the return.

google/cloud/firestore_v1/base_document.py Outdated Show resolved Hide resolved
google/cloud/firestore_v1/base_document.py Outdated Show resolved Hide resolved
google/cloud/firestore_v1/document.py Show resolved Hide resolved
google/cloud/firestore_v1/transaction.py Show resolved Hide resolved
@crwilcox crwilcox merged commit 30bb3fb into googleapis:master Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/python-firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve type information from Any
3 participants