1Password/connect-sdk-python

Add option to enable / disable async to new_client_from_env()

mjpieters opened this issue ยท 4 comments

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:

  • set the ENV_IS_ASYNC_CLIENT environment variable manually before calling new_client_from_environment(). This feels very, very janky. I should not have to manipulate the environment variables visible to a 3rd-party SDK to switch between async modes for my code base.
  • use new_client() directly but then miss out on the helpful validation that new_client_from_environment() also provides.

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.

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

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! ๐Ÿ˜„

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.