[Storage] [Named Keywords] File Share Package #42100
Conversation
weirongw23-msft
requested review from
annatisch,
jalauzon-msft and
vincenttran-msft
as code owners
There was a problem hiding this comment.
Pull Request Overview
This pull request merges the File Share Named Keywords branch into main, introducing comprehensive type stubs (.pyi files) for the Azure Storage File Share SDK's async client components and updating the synchronous lease client implementation. The changes enhance type safety and developer experience through improved type annotations and code quality improvements.
Key changes include:
- Addition of complete type stub files for async client classes
- Type safety improvements in the lease client implementation
- Enhanced code formatting and consistency across the codebase
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
aio/_share_service_client_async.pyi |
New type stub for async share service client with comprehensive method signatures |
aio/_share_client_async.pyi |
New type stub for async share client with all share management operations |
aio/_lease_async.pyi |
New type stub for async lease client operations |
aio/_lease_async.py |
Updated implementation with improved formatting and type safety |
aio/_file_client_async.pyi |
New type stub for async file client with file operations |
aio/_directory_client_async.pyi |
New type stub for async directory client with directory operations |
_share_service_client.pyi |
New type stub for sync share service client |
_share_client.pyi |
New type stub for sync share client |
_lease.pyi |
New type stub for sync lease client |
_lease.py |
Updated implementation with type safety improvements |
_file_client.pyi |
New type stub for sync file client |
_directory_client.pyi |
New type stub for sync directory client |
Comments suppressed due to low confidence (2)
sdk/storage/azure-storage-file-share/azure/storage/fileshare/_file_client.pyi:276
- Parameter naming inconsistency: sync version uses 'if_modified_since' while async version uses 'source_if_modified_since'. This could cause confusion for developers switching between sync and async versions.
if_modified_since: Optional[datetime] = None,
sdk/storage/azure-storage-file-share/azure/storage/fileshare/aio/_lease_async.py
Show resolved
Hide resolved
|
|
||
| if TYPE_CHECKING: | ||
| from datetime import datetime | ||
| from azure.storage.fileshare import ShareClient, ShareFileClient | ||
|
|
||
|
|
||
| class ShareLeaseClient(object): # pylint: disable=client-accepts-api-version-keyword | ||
| class ShareLeaseClient: # pylint: disable=client-accepts-api-version-keyword |
There was a problem hiding this comment.
Removing '(object)' inheritance is good for Python 3, but ensure this change is consistent across the entire codebase.
Copilot uses AI. Check for mistakes.
| self, *, lease: Optional[Union[ShareLeaseClient, str]] = None, timeout: Optional[int] = None, **kwargs: Any | ||
| ) -> None: ... | ||
| @distributed_trace | ||
| def rename_file( |
There was a problem hiding this comment.
The sync version of rename_file has different keyword-only parameter structure compared to the async version. The async version uses '*,' before timeout while sync version doesn't. This inconsistency could confuse API consumers.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Yeah, that ^ I think you are missing the *, but also, the parameters are different i.e. missing timeout?
There was a problem hiding this comment.
This is a really good catch, turns out the * was missing to separate positional and keyword arguments. There is currently no timeout documented. However, to stay consistent with other APIs we'll put it here because it is technically accepted.
weirongw23-msft
changed the title
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
| ) -> None: | ||
| self.id = lease_id or str(uuid.uuid4()) | ||
| self.last_modified = None | ||
| self.etag = None | ||
| if hasattr(client, 'file_name'): | ||
| if hasattr(client, "file_name"): |
There was a problem hiding this comment.
Same broadly applicable comment here-- seems like you only ran black formatter on async? Just be consistent between sync and async (all or nothing)
There was a problem hiding this comment.
Yeah again this was weird so I'll revert the changes for lease. The other files all look fine. Don't know why only lease got formatted (or I thought it was a good idea :))
Will do the black formatting in a separate PR!
No description provided.