[Storage] [Named Keywords] [Blob] _container_client.py and aio
#40692
Conversation
…ntainer-client-nk
weirongw23-msft
changed the base branch from
main
to
feature/storage-blob-named-keywords
weirongw23-msft
requested review from
annatisch,
jalauzon-msft and
vincenttran-msft
as code owners
| @@ -264,3 +272,33 @@ def _generate_set_tiers_options( | |||
| reqs.append(req) | |||
|
|
|||
| return reqs, kwargs | |||
|
|
|||
|
|
|||
| def _delete_container_options(**kwargs: Any) -> Dict[str, Any]: | |||
There was a problem hiding this comment.
Could theoretically get rid of this and do this in line like some of the APIs. We'll have to decide which one we like more in container client (and in blob generally) since there is not a lot for those options but it is cleaner.
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
weirongw23-msft
changed the base branch from
feature/storage-blob-named-keywords
to
main
weirongw23-msft
changed the base branch from
main
to
feature/storage-blob-named-keywords
|
TODO: Remove docstring-should-match-keywords only |
| :keyword str secondary_hostname: | ||
| The hostname of the secondary endpoint. |
There was a problem hiding this comment.
nit: Could you move audience docstring up here? Not sure if it's worth doing a sweeping change (or if other APIs even follow this), but it would be cool if the docstrings could appear in the same order as the named keywords
There was a problem hiding this comment.
Looking closer-- I also realized the other new docstrings you added are out of order as well. Could you order docstring to match the named keywords ordering? You may also want to retroactively apply this to Blobs.. (sorry for not catching this earlier)
| :keyword str secondary_hostname: | ||
| The hostname of the secondary endpoint. | ||
| :keyword int max_block_size: The maximum chunk size for uploading a block blob in chunks. |
There was a problem hiding this comment.
Same nit here, if we could move audience docstring between these two docstrings -- once again just to match named keywords appearing with docstring ordering
EDIT: And again, if you could order the other keyword docstrings to match the named keywords ordering
| timeout = kwargs.pop('timeout', None) | ||
| headers.update(add_metadata_headers(metadata)) # type: ignore | ||
| container_cpk_scope_info = get_container_cpk_scope_info(kwargs) | ||
| headers.update(add_metadata_headers(metadata)) # type: ignore |
There was a problem hiding this comment.
Is there any more specific ignores here we could apply instead of the all mighty 🔨 ? Fine to leave if there isn't a better alternative.
| try: | ||
| return self._client.container.create( # type: ignore | ||
| return self._client.container.create( # type: ignore |
There was a problem hiding this comment.
Same comment here- is there a more specific ignore or is there multiple errors we are suppressing here?
| @@ -473,10 +620,18 @@ def acquire_lease( | |||
| :dedent: 8 | |||
| :caption: Acquiring a lease on the container. | |||
| """ | |||
| lease = BlobLeaseClient(self, lease_id=lease_id) # type: ignore | |||
| lease = BlobLeaseClient(self, lease_id=lease_id) # type: ignore | |||
There was a problem hiding this comment.
nit: Any more specific ignores?
| if etag is not None: | ||
| kwargs['etag'] = etag |
There was a problem hiding this comment.
Is there a specific reason why we need etag to be populated in the kwargs bag and not passed directly as a named keyword?
| identifiers.append(SignedIdentifier(id=key, access_policy=value)) # type: ignore | ||
| signed_identifiers = identifiers # type: ignore |
There was a problem hiding this comment.
same nit: Any more specific ignores?
| secondary_hostname: Optional[str] = None, | ||
| audience: Optional[str] = None, | ||
| max_block_size: int = 4 * 1024 * 1024, | ||
| max_page_size: int = 4 * 1024 * 1024, | ||
| max_chunk_get_size: int = 4 * 1024 * 1024, | ||
| max_single_put_size: int = 64 * 1024 * 1024, | ||
| max_single_get_size: int = 32 * 1024 * 1024, | ||
| min_large_block_upload_threshold: int = 4 * 1024 * 1024 + 1, | ||
| use_byte_buffer: Optional[bool] = None, |
There was a problem hiding this comment.
same nit: If we could order the docstring to match the named keywords (or vice versa, just keep it consistent) and probably would be good to keep it consistent b/w sync and async clients, and this laundry list of options should appear in the same ordering if it appears in any other APIs
No description provided.