[Storage] Decoupled Client Context Manager Methods from Base Client #41442
Conversation
|
/azp run python - pullrequest |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Placed context managers directly after constructors, removed those implementations from base client (sync / async) |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews azure-storage-blob |
There was a problem hiding this comment.
Pull Request Overview
This pull request decouples context manager methods from the base client into individual client implementations. The key changes include adding enter, exit, and close methods to various synchronous and asynchronous client classes, and removing these context manager methods from shared base classes.
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_share_service_client.py | Added sync context manager methods (enter, exit, close) |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_share_client.py | Added sync context manager methods with pylint disable comment for too-many-public-methods |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_file_client.py | Added sync context manager methods |
| sdk/storage/azure-storage-file-share/azure/storage/fileshare/_directory_client.py | Added sync context manager methods |
| sdk/storage/azure-storage-file-datalake/azure/storage/filedatalake/aio/* | Added async context manager methods and adjusted generated client building |
| sdk/storage/azure-storage-file-datalake/azure/storage/filedatalake/_shared/* | Removed context manager methods from shared base classes |
| sdk/storage/azure-storage-blob/azure/storage/blob/aio/* | Added async context manager methods |
| sdk/storage/azure-storage-blob/azure/storage/blob/* | Added sync context manager methods |
sdk/storage/azure-storage-file-share/azure/storage/fileshare/_share_service_client.py
Show resolved
Hide resolved
| It need not be used when using with a context manager. | ||
|
|
||
| :return: None | ||
| :rtype: None |
There was a problem hiding this comment.
Can we be consistent with the docs between sync/async? Here we document the return type, in sync we don't. We probably don't need to too IMO but it's up to you.
| @@ -126,13 +126,13 @@ def __init__( | |||
| self._loop = kwargs.get('loop', None) | |||
|
|
|||
| async def __aenter__(self) -> Self: | |||
| await super(DataLakeServiceClient, self).__aenter__() | |||
| await self._client.__aenter__() | |||
| await self._blob_service_client.__aenter__() | |||
There was a problem hiding this comment.
Hmm this is the only place we call enter/aenter on a internal blob client like this. I don't think we need to do that. Calling close on exit should be enough.
| @@ -190,6 +190,19 @@ def __init__( | |||
| self._client._config.version = get_api_version(kwargs) # type: ignore [assignment] | |||
| self._configure_encryption(kwargs) | |||
|
|
|||
| def __enter__(self): | |||
There was a problem hiding this comment.
Let's at least document the type for enter since that is an important one. Probably just -> Self? This applies to the whole review.
For exit, maybe we add the return type -> None but leave the *args empty to be filled in in the pyi files.
| @@ -120,6 +120,19 @@ def __init__( | |||
| self._client._config.version = get_api_version(api_version) # type: ignore [assignment] | |||
| self._configure_encryption(kwargs) | |||
|
|
|||
| def __enter__(self): | |||
There was a problem hiding this comment.
Queue doesn't have pyi files so I guess we need to document the full type for enter/exit here. Reminder this is the full proper type hint for exit:
https://github.com/python/typeshed/blob/567b488fc28978642bfeb70b29f11453b0ff124c/stdlib/imp.pyi#L48
No description provided.