-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consistent returns #13245
Consistent returns #13245
Conversation
…gly and reran tests
sdk/tables/azure-data-tables/azure/data/tables/_table_client.py
Outdated
Show resolved
Hide resolved
sdk/tables/azure-data-tables/azure/data/tables/_table_client.py
Outdated
Show resolved
Hide resolved
row_key=row_key, | ||
if_match=if_match or if_not_match or "*", | ||
table_entity_properties=entity, | ||
cls=kwargs.pop('cls', _return_headers_and_deserialized), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@annatisch had mentioned before that she prefers something more like
cls=kwargs.pop('cls', None) or _return_headers_and_deserialized
(can't remember this comment correctly, I think it might have had something to do with if the user passes in None
for cls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way it currently is if the user passes in None
that will be passed to the service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wont be passed to the service - as this parameter deals with client-side behaviour.
But yeah... this one is tricky, because if a user does provide a cls
parameter, and it doesn't return a tuple of two values - the function will fail.....
…rrect response instead that it is not None
sdk/tables/azure-data-tables/azure/data/tables/_table_client.py
Outdated
Show resolved
Hide resolved
sdk/tables/azure-data-tables/azure/data/tables/_table_client.py
Outdated
Show resolved
Hide resolved
:raises: ~azure.core.exceptions.HttpResponseError | ||
""" | ||
table_properties = TableProperties(table_name=self.table_name, **kwargs) | ||
try: | ||
table = self._client.table.create(table_properties) | ||
return TableItem(table=table) | ||
metadata, _ = self._client.table.create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's in this metadata? Do we need to clean out unnecessary information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The returned metadata has these keys: client_request_id
, request_id
, version
, date
, and etag
always. On create_entity
the service also returns preference_applied
and content_type
. client_request_id
, request_id
, and preference_applied
are all None
in the tests
sdk/tables/azure-data-tables/azure/data/tables/_table_client.py
Outdated
Show resolved
Hide resolved
sdk/tables/azure-data-tables/azure/data/tables/aio/_table_client_async.py
Outdated
Show resolved
Hide resolved
match_condition=kwargs.pop('match_condition', None)), | ||
etag_param='etag', match_param='match_condition') | ||
try: | ||
self._client.table.delete_entity( | ||
table=self.table_name, | ||
partition_key=partition_key, | ||
row_key=row_key, | ||
if_match=if_match or if_not_match or '*', | ||
if_match=if_match or '*', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a question: why are you no longer checking if_not_match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if_not_match
is not supported by the service right now.
self.assertIn("version", keys) | ||
self.assertIn("date", keys) | ||
self.assertIn("etag", keys) | ||
self.assertEqual(len(keys), 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to add a test that there's no other metadata besides version, date, and etag? Also wondering how these three were the ones decided on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is other metadata but it is intentionally stripped away by @annatisch 's recommendation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see, but can you test that the stripped away metadata isn't present in the final response object, or am I misunderstanding what actually gets stripped away
…into expose_parse_vault_id * 'master' of https://github.com/Azure/azure-sdk-for-python: (37 commits) [text analytics] add versionadded sphinx documentation (Azure#13450) [text analytics] add bing_id property to LinkedEntity class (Azure#13446) fix typing for paging methods (Azure#13410) [text analytics] add domain_filter param (Azure#13451) fix issue Azure#11658 for is_valid_resource_id (Azure#11709) added create_table_if_not_exists method to table service client (Azure#13385) [ServiceBus] Test and failure improvements (Azure#13345) Proper encoding and decoding of source URLs - Fixes special characters in source URL issue (Azure#13275) Switch retry (Azure#13264) [ServiceBus] ServiceBusClient close spawned children (Azure#13077) fixing version issue by not overwriting the version with the semantic… (Azure#13411) clean up reference and tests (Azure#13412) Consistent returns (Azure#13245) [text analytics] return None for offset and length for v3.0 (Azure#13382) edit all authentication files and add a test (Azure#13355) Add managed_identity_client_id argument to DefaultAzureCredential (Azure#13218) [text analytics] add string-index-type support (Azure#13378) [text analytics] fix error response if pii entities is called from v3.0 client (Azure#13383) Send spec (Azure#13143) Anomaly Detector 3.0.0b2 release (Azure#13351) ...
Addresses #13239
Creates a consistent response for table client methods. Changing from an entity to a headers response with the eTag