1Password / connect-sdk-python

Python SDK for 1Password Connect
https://developer.1password.com/docs/connect
MIT License
200 stars 30 forks source link

Add option to enable / disable async to new_client_from_env() #84

Open mjpieters opened 11 months ago

mjpieters commented 11 months ago

Summary

Allow the program to decide on async vs sync client use when loading the credentials from the environment.

Use cases

Whether or not an async client is needed is a choice for the specific application code, not the environment. Please allow me to load an async client in an async codebase, and a sync client in a sync codebase:

def sync_main():
    client = new_client_from_env(is_async=False)

async def async_main():
    client = new_client_from_env(is_async=True)

Proposed solution

Add a new is_async keyword argument to new_client_from_env(), defaulting to None. If it is left to the default, the environment variable is used, otherwise the value is passed on directly to new_client():

from typing import Optional

# ...

def new_client_from_environment(
    url: Optional[str] = None, token: Optional[str] = None, is_async: Optional[bool] = None
):
    # ...
    if is_async is None:
        is_async = os.environ.get(ENV_IS_ASYNC_CLIENT).lower() in ("t", "true", "1")
    # ...
    return new_client(url, token, is_async=is_async)

(I made the test for ENV_IS_ASYNC_CLIENT a little broader to also accept T, true, 1, etc, as signalling 'true')

Is there a workaround to accomplish this today?

I have to options right now:

mjpieters commented 11 months ago

There is another reason why this change is beneficial: type checking.

Currently, a type checker can't know what kind of client is being returned from the new_client_from_environment() call, as the object can be either a Client or a AsyncClient instance. With the right typing.overload annotations, type checkers can explicitly be told what kind of client to expect. This makes working with this library much, much easier in supporting development environments.

volodymyrZotov commented 11 months ago

Hi, @mjpieters! Thank you for raising this πŸ‘. That's definitely beneficial to implement. We'll add this to our to-do list.

mjpieters commented 11 months ago

Hi, @mjpieters! Thank you for raising this πŸ‘. That's definitely beneficial to implement. We'll add this to our to-do list.

I had already created a PR for this but neglected to reference this issue in the PR. So, if you want, the todo item on your list can be marked as 'done' if you choose to merge that PR! πŸ˜„

volodymyrZotov commented 11 months ago

That's great! I'll take a look πŸ‘ Thank you very much for this and other PRs you opened! I'm going to review them all.