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

Incorrect type hints in collection() #289

Closed
mbrancato opened this issue Jan 9, 2021 · 3 comments · Fixed by #300
Closed

Incorrect type hints in collection() #289

mbrancato opened this issue Jan 9, 2021 · 3 comments · Fixed by #300
Assignees
Labels
api: firestore Issues related to the googleapis/python-firestore API. help wanted We'd love to have community involvement on this issue. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@mbrancato
Copy link

mbrancato commented Jan 9, 2021

Environment details

  • OS type and version: MacOS
  • Python version: Python 3.7.7
  • google-cloud-firestore version: 2.0.2

At some point recently the type hints for for Client().collection() methods were changed, and they do not reflect examples or the docs. I'm getting type hint warnings.

def collection(self, *collection_path: Tuple[str]) -> CollectionReference:

Should the type hint be changed to simply str ? All the comments and docs show these:

client.collection('top')
client.collection('mydocs/doc/subcol')
client.collection('mydocs', 'doc', 'subcol')

None of these are Tuple[str]. The argument *collection_path will produce a tuple when called, but it is called with strings.

@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/python-firestore API. label Jan 9, 2021
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jan 10, 2021
@dmahugh dmahugh added help wanted We'd love to have community involvement on this issue. and removed triage me I really want to be triaged. labels Jan 11, 2021
@dmahugh dmahugh assigned dmahugh and craiglabenz and unassigned dmahugh Jan 11, 2021
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jan 11, 2021
@dmahugh dmahugh added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Jan 12, 2021
@dmahugh dmahugh assigned tseaver and unassigned craiglabenz Jan 29, 2021
@tseaver
Copy link
Contributor

tseaver commented Feb 2, 2021

Per the implementation, the collection_path argument can be either a single, slash-delimited path string, or a Tuple[str]. See the _path_helper function, which also has an incorrect typespec for its argument.

@tseaver
Copy link
Contributor

tseaver commented Feb 2, 2021

A correction: the leading asterisk (*collection_path) means that the value received by the function will always be a tuple, and we require that the elements of the tuple be strings. The caller may pass a single string, e.g. client.collection("mydocs/doc/subcol"), which will be split on the slashes, exactly as if the caller had passed client.collection("mydocs", "doc", "subcol"). That example is from the docstring of Client.collection.

@tseaver
Copy link
Contributor

tseaver commented Feb 2, 2021

OK, looking at PEP 484, it appears that *args should be typed as the type of one element.

tseaver added a commit that referenced this issue Feb 2, 2021
PEP 484 specifies that they be hinted as the type of a single element,
as seen from the caller's perspective.

Closes #289.
tseaver added a commit that referenced this issue Feb 2, 2021
PEP 484 specifies that they be hinted as the type of a single element,
as seen from the caller's perspective.

Closes #289.
crwilcox added a commit that referenced this issue Feb 4, 2021
PEP 484 specifies that they be hinted as the type of a single element,
as seen from the caller's perspective.

Closes #289.

Co-authored-by: Christopher Wilcox <crwilcox@google.com>
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. help wanted We'd love to have community involvement on this issue. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants