Add Typing to args and class attributes

[thehesiod]

[jupiterbjy]

https://github.com/omarryhan/aiogoogle/blob/4eef158c92ca710591322fbe5d2a89a2fae6c52a/examples/auth/oauth2.py#L28-L41

I can see Aiogoogle.client_creds was meant to accept the dict.

However, in docstring:

https://github.com/omarryhan/aiogoogle/blob/4eef158c92ca710591322fbe5d2a89a2fae6c52a/aiogoogle/client.py#L30-L36

Which then makes linter to give false warning like below:

Previously I thought adding Union[] on docstring would be enough, turns out PEP-484 actually does not mention anything about use of typing.Union inside docsting, and therefore not properly supported by IDEs.

So only viable option would be making proper annotations, I'll try to make PL if I manage to annotate it later on.

[omarryhan]

Hmmm, interesting. The reason I added the ClientCreds model in the docstring in the first place was to have it documented in the docs. The ClientCreds model inherits from dict. Probably not best practice. But I'm open to changing it as long as everything is backwards compatible. Thanks!

[omarryhan]

Yeah feel free to make this change. As long as getitem/setitem methods work and values can be accessed using this syntax user_dict['item_name'].

I think the UserDict object didn't exist in Python when I first wrote this lib. Not sure though. If UserDict isn't supported in py3.6, then I'm not sure this change is worth it? 🤔

On Wed, Jan 19, 2022, 13:35 jupiterbjy @.***> wrote:

Just looked up implementation of _dict, have any plan to use collections.UserDict https://docs.python.org/3.7/library/collections.html#collections.UserDict instead? As far as I know subclassing of built-in types like dict, list etc in CPython is not recommended, hence there's built-in alternatives for Dict that are designed to be subclassed without hassle.

— Reply to this email directly, view it on GitHub https://github.com/omarryhan/aiogoogle/issues/16#issuecomment-1016374430, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGPFI5J77VDSHLA63BAQXTTUW2OXJANCNFSM4KF2AKOQ . You are receiving this because you modified the open/close state.Message ID: @.***>

[jupiterbjy]

Well... Seems like best bet is to add Type Annotation and keeping type hint in docstring at same time, as definition part get dirty really quickly.

Above result is from code change of following:

class Aiogoogle:
    # ...
    """
    Arguments:

        session_factory (aiogoogle.sessions.abc.AbstractSession): AbstractSession Implementation. Defaults to ``aiogoogle.sessions.aiohttp_session.AiohttpSession``

        api_key (aiogoogle.auth.creds.ApiKey | str): Google API key

        user_creds (aiogoogle.auth.creds.UserCreds | Mapping): OAuth2 cser credentials

        client_creds (aiogoogle.auth.creds.ClientCreds | Mapping): OAuth2 client credentials

        service_account_creds (aiogoogle.auth.creds.ServiceAccountCreds | Mapping): Service account credentials
    """

    def __init__(
        self,
        session_factory: Type[AbstractSession] = AiohttpSession,
        api_key: Union[ApiKey, str] = None,
        user_creds: Union[UserCreds, Mapping] = None,
        client_creds: Union[ClientCreds, Mapping] = None,
        service_account_creds: Union[ServiceAccountCreds, Mapping] = None,
    ):

And this do fix false warning when passing dict.

---

I'm not sure if cleaner alternative is possible, considering other projects that's using readthedocs.org are also having very complex-looking argument page - They decided to keep both type hints on type annotation and docstring.

So yeah, I'll say it's totally possible to make it backward-compatible but also I'm no expert!

I'm leaving final decision up to you!

[jupiterbjy]

Yeah feel free to make this change. As long as getitem/setitem methods work and values can be accessed using this syntax user_dict['item_name'].

Actually I thought about it again and reached conclusion "Why fix when there's no issue?" - So I removed that comment.

I think the UserDict object didn't exist in Python when I first wrote this lib. Not sure though. If UserDict isn't supported in py3.6, then I'm not sure this change is worth it? 🤔

As far as I know it was added on Python 3.7, depending on when you created this repository it might not existed!

[omarryhan]

The changes you made to the Aiogoogle class look very good to me. I think it's worth it to litter the blue part of the docs if it gives us better typing in IDEs. Most people will be looking at the parameters section anyways.

[omarryhan]

As far as I know it was added on Python 3.7, depending on when you created this repository it might not existed!

Let's stick with supporting Py3.6 then.

"Why fix when there's no issue?"

Agreed

[jupiterbjy]

Ah wasn't it targeting 3.7? Somehow aiogoogle on pypi is reporting 3.7

[omarryhan]

Pypi is missing some targets it seems. In the test suite (Tox) and Github Actions we're testing for 3.6.

e.g. here https://github.com/omarryhan/aiogoogle/blob/master/.github/workflows/action.yml#L56 and here: https://github.com/omarryhan/aiogoogle/blob/master/tox.ini#L2

[jupiterbjy]

I checked and it was I who was being dumb; changes I mentioned are totally unnecessary, it was me who was feeding wrong dict structure (Feeding string to scope, not list) to Aiogoogle, sorry!

[omarryhan]

Totally okay! :)

On 21 Jan 2022, at 12:42 PM, jupiterbjy @.***> wrote:

I checked and it was I who was being dumb; changes I mentioned are totally unnecessary, it was me who was feeding wrong dict structure (Feeding string to scope, not list) to Aiogoogle, sorry!

— Reply to this email directly, view it on GitHub https://github.com/omarryhan/aiogoogle/issues/16#issuecomment-1018390188, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGPFI5IKZEOAKX7GXUZB5JTUXE2CPANCNFSM4KF2AKOQ. You are receiving this because you modified the open/close state.