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

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

Open
mjpieters opened this issue Oct 6, 2023 · 4 comments · May be fixed by #89
Open

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

mjpieters opened this issue Oct 6, 2023 · 4 comments · May be fixed by #89

Comments

@mjpieters
Copy link
Contributor

mjpieters commented Oct 6, 2023

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.
@mjpieters
Copy link
Contributor Author

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
Copy link
Contributor

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

@mjpieters mjpieters linked a pull request Oct 9, 2023 that will close this issue
@mjpieters
Copy link
Contributor Author

mjpieters commented Oct 9, 2023

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants