[Storage] [Named Keywords] [Blob] _blob_client.py and aio
#40206
Conversation
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
|
/azp run python - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… or default_value when default_value != None
|
/azp run python - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
weirongw23-msft
requested review from
annatisch,
jalauzon-msft and
vincenttran-msft
as code owners
sdk/storage/azure-storage-blob/azure/storage/blob/_blob_client.py
Outdated
Show resolved
Hide resolved
| 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.
Making a comment here although I know we have had backs and forths- my stance is that I think these could stay, but again I think either way is fine.
There was a problem hiding this comment.
Also good with letting this stay for now :)
There was a problem hiding this comment.
__init__, from_blob_url, from_connection_string
| raise ValueError("Customer provided encryption key must be used over HTTPS.") | ||
| options = _download_blob_options( | ||
| blob_name=self.blob_name, | ||
| container_name=self.container_name, | ||
| version_id=get_version_id(self.version_id, kwargs), |
There was a problem hiding this comment.
Not sure if this is the only instance-- but eventually could delete this helper once unused
There was a problem hiding this comment.
Yup made a note of this! This will be deleted once I go thru container client and service client.
| @@ -2407,7 +3064,7 @@ def get_page_ranges( | |||
| :returns: | |||
| A tuple of two lists of page ranges as dictionaries with 'start' and 'end' keys. | |||
| The first element are filled page ranges, the 2nd element is cleared page ranges. | |||
| :rtype: tuple(list(dict(str, str), list(dict(str, str)) | |||
| :rtype: tuple(list(Dict[str, str], list(Dict[str, str]) | |||
There was a problem hiding this comment.
nit: Can we do capitals on Tuple and List? Also any other instances in this file
| validate_content = kwargs.pop('validate_content') or False | ||
| content_settings = kwargs.pop('content_settings') or None | ||
| overwrite = kwargs.pop('overwrite') or False | ||
| max_concurrency = kwargs.pop('max_concurrency') or 1 | ||
| cpk = kwargs.pop('cpk') or None |
There was a problem hiding this comment.
What's the reasoning for this? Assuming maybe something with type of default valued kwargs.pop()?
There was a problem hiding this comment.
This is not strictly needed for cpk or content_settings, or anytime we do kwargs.pop('kwarg', None). The idea behind doing this is for example kwargs.pop('max_concurrency', 1), if max_concurrency=None, it will default to None instead of 1.
| @@ -687,7 +734,7 @@ def _stage_block_options( | |||
| ) -> Dict[str, Any]: | |||
| block_id = encode_base64(str(block_id)) | |||
| if isinstance(data, str): | |||
| data = data.encode(kwargs.pop('encoding', 'UTF-8')) # type: ignore | |||
| data = data.encode(kwargs.pop('encoding') or 'UTF-8') # type: ignore | |||
There was a problem hiding this comment.
same q as above and on L746-- fine to resolve if there is a reason (but also, can we get a more specific ignore here or is there multiple errors 😄 )
There was a problem hiding this comment.
Yeah if encoding is None, it will default to None instead of UTF-8 which is what we want. This fix actually removed the need for a type ignore, so I'll remove that :)
| if offset is None or offset % 512 != 0: | ||
| raise ValueError("offset must be an integer that aligns with 512 page size") | ||
| if length is None or length % 512 != 0: | ||
| raise ValueError("length must be an integer that aligns with 512 page size") | ||
| end_range = offset + length - 1 # Reformat to an inclusive range index | ||
| content_range = f'bytes={offset}-{end_range}' # type: ignore | ||
| content_range = f'bytes={offset}-{end_range}' # type: ignore |
There was a problem hiding this comment.
1.) Is this ignore still needed
2.) Can we make it more specific?
There was a problem hiding this comment.
Nice catch! We don't need this
There was a problem hiding this comment.
Went thru all the files to either remove redundant ignores or made them more specific :)
| @@ -121,14 +121,17 @@ class BlobServiceClient(StorageAccountHostsMixin, StorageEncryptionMixin): | |||
| def __init__( | |||
| self, account_url: str, | |||
| credential: Optional[Union[str, Dict[str, str], "AzureNamedKeyCredential", "AzureSasCredential", "TokenCredential"]] = None, # pylint: disable=line-too-long | |||
| *, | |||
| api_version: Optional[str] = None, | |||
| # TODO | |||
There was a problem hiding this comment.
Could remove this TODO if you want 😄 or leave it since it's going into a feature branch
There was a problem hiding this comment.
Nah I'll leave it so I know where to start <3
|
/azp run python - pullrequest |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Edit: With the number of named keywords added,
pylintis recognizing them as local variables. We have APIs that exceed the allowed amount of 25. For now, I've suppressed the error.