googleapis / python-firestore

Apache License 2.0
212 stars 72 forks source link

Incorrect type annotation(s) in AsyncTransaction constructor #537

Open anna-hope opened 2 years ago

anna-hope commented 2 years ago

The type annotation for client in AsyncTransaction constructor should be ...async_client.AsyncClient, not ...client.Client: https://github.com/googleapis/python-firestore/blob/3f1fd365688980c9f82a9fc69650129fa8c01dcf/google/cloud/firestore_v1/async_transaction.py#L56

I can submit a PR to fix this.

I am not sure if the annotation for _commit_with_retry https://github.com/googleapis/python-firestore/blob/3f1fd365688980c9f82a9fc69650129fa8c01dcf/google/cloud/firestore_v1/async_transaction.py#L346 should also refer to AsyncClient. If it should, I can fix that too.

t-kozak commented 2 years ago

The type annotations in general are problematic throughout the API. For instance, the AsyncQuery#get method:

async def get(
        self,
        transaction: Transaction = None,
        retry: retries.Retry = gapic_v1.method.DEFAULT,
        timeout: float = None,
    ) -> list:

the transaction here is not market as Optional[Transaction], which causes type checker configured to strict to complain that None is invalid value.

Additionally, in comment of this method:

transaction
                (Optional[:class:`~google.cloud.firestore_v1.transaction.Transaction`]):

the transaction is supposed to be an Optional[Transaction], but not an AsyncTransaction - when you use AsyncClient, you get AsyncTransaction.

As a general note, given the popularity of VisualStudio Code, and thus pylance type checker, it would be awesome if you folks could allocate a day or two of someone's work life to just go through the APIs documentation and annotation and double check that simple use of the library does not involve frequent use of # type:ignore.

Mariatta commented 1 year ago

Indeed there are mistyped annotations that should be fixed. And at the moment there are 168 uses of # type: ignore so this will take some time to address.

meredithslota commented 11 months ago

Related to #447