Skip to content
This repository was archived by the owner on Mar 2, 2026. It is now read-only.

feat: improve type information#176

Merged
crwilcox merged 19 commits into
googleapis:masterfrom
MaxxleLLC:firestore_improve_type
Oct 23, 2020
Merged

feat: improve type information#176
crwilcox merged 19 commits into
googleapis:masterfrom
MaxxleLLC:firestore_improve_type

Conversation

@HemangChothani

Copy link
Copy Markdown
Contributor

Fixes #139

@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Aug 27, 2020

@tseaver tseaver left a comment

Copy link
Copy Markdown
Contributor

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

tseaver commented Sep 16, 2020

Copy link
Copy Markdown
Contributor

Blocked on #188.

@tseaver tseaver removed the status: blocked Resolving the issue is dependent on other work. label Sep 24, 2020

@tseaver tseaver left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread google/cloud/firestore_v1/_helpers.py Outdated
Comment thread google/cloud/firestore_v1/base_document.py Outdated
Comment thread google/cloud/firestore_v1/document.py Outdated
Comment thread google/cloud/firestore_v1/document.py Outdated
Comment thread google/cloud/firestore_v1/document.py Outdated
Comment thread google/cloud/firestore_v1/transaction.py Outdated
Comment thread google/cloud/firestore_v1/transaction.py Outdated
@crwilcox

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@tseaver PTAL

@crwilcox

crwilcox commented Oct 20, 2020

Copy link
Copy Markdown
Contributor

@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.

Comment thread google/cloud/firestore_v1/base_document.py
Comment thread google/cloud/firestore_v1/base_document.py
Comment thread google/cloud/firestore_v1/document.py
Comment thread google/cloud/firestore_v1/transaction.py
Comment thread google/cloud/firestore_v1/_helpers.py
@crwilcox
crwilcox merged commit 30bb3fb into googleapis:master Oct 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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