Closed alexgleith closed 11 months ago
I probably should've given you a chance to approve me moving that azure stuff in there. I think this is fine for import, but if None is passed instead it will obv cause an error. Absent a better solution, is it ok to just test for none and raise the exception here?
Yeah, I know it'll raise and exception, but that's when a function is called instead of on import.
I can add a check for None in the function if you like?
Or could move the environment variable getting into there too... maybe that's better.
It actually doesn't appear to raise an exception until later (an invalid container client is returned). I think it'd be better to raise the error so the default value is still in the function sig. But for just us using it that may be pedantic. How about just
def get_container_client(
storage_account: str = os.environ.get("AZURE_STORAGE_ACCOUNT"),
container_name: str = "output",
credential: str = os.environ.get("AZURE_STORAGE_SAS_TOKEN"),
) -> ContainerClient:
if storage_account is None:
raise ValueError("'None' is not a valid value for 'storage_account'. Pass a valid name or set the 'AZURE_STORAGE_ACCOUNT' environment variable")
# same for credential...
return azure.storage.blob.ContainerClient(
f"https://{storage_account}.blob.core.windows.net",
container_name=container_name,
credential=credential,
)
Yep, that works for me.
We're the arguments are evaluated on import, but not again until you use the function!
Do a
os.environ.get("EXAMPLE")
instead ofos.environ["EXAMPLE"]
, which means there's a default ofNone
retrieved and failure is less immediate and fatal on importing function.