Skip to content
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

raise decode error instead of ContentDecodingError #19433

Merged
merged 4 commits into from
Nov 10, 2021

Conversation

xiangyan99
Copy link
Member

No description provided.

@ghost ghost added the Azure.Core label Jun 24, 2021
@xiangyan99 xiangyan99 marked this pull request as ready for review June 24, 2021 18:04
@xiangyan99 xiangyan99 requested a review from lmazuel as a code owner June 24, 2021 18:04
@@ -2,6 +2,9 @@

## 1.15.1 (Unreleased)

### Breaking Changes

- Sync stream downloading now raises `azure.core.exceptions.DecodeError` rather than `requests.exceptions.ContentDecodingError`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we okay with this breaking change because no one should have written code to explicitly handle this error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Checked with @johanste and he agreed this was an acceptable breaking change.

raise requests.exceptions.ContentDecodingError(e)
raise ServiceResponseError(e, error=e)
except CoreDecodeError as e:
raise DecodeError(e, error=e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scenario is being correctly handled in the other transports?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently different transport raises different type of error. (That's the reason we need this change).

We want to uniform the errors so user can catch all by using except AzureError.

@xiangyan99
Copy link
Member Author

/azp run python - core - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-cosmos. You can review API changes here

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-core. You can review API changes here

@xiangyan99 xiangyan99 merged commit 4205ea9 into main Nov 10, 2021
@xiangyan99 xiangyan99 deleted the core_stream_decode_error branch November 10, 2021 01:07
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Nov 10, 2021
…into add_webpubsub_tests

* 'main' of https://github.com/Azure/azure-sdk-for-python:
  [Key Vault] Add support for multi-tenant authentication (Azure#21290)
  [webpubsub] regen with hub as a client parameter (Azure#21688)
  update automatic close mechanism (Azure#21580)
  [Test Proxy] Add fixture to automatically start/stop Docker container (Azure#21538)
  Update Monitor Query API ref link (Azure#21683)
  Migration Guide from Azure-loganalytics (Azure#21674)
  Update docs for Web PubSub GA (Azure#21659)
  Update CHANGELOG.md (Azure#21681)
  Increment version for formrecognizer releases (Azure#21678)
  Increment version for videoanalyzer releases (Azure#21455)
  Increment version for cognitivelanguage releases (Azure#21566)
  Increment version for storage releases (Azure#21652)
  Increment version for communication releases (Azure#21667)
  raise decode error instead of ContentDecodingError (Azure#19433)
  Update CHANGELOG.md (Azure#21679)
  resolve mac agent failure (Azure#21677)
  Re-add get-codeowners.ps1 (Azure#21676)
  [SchemaRegistry] remove schema prefix in params (Azure#21675)
  Validate python docs packages using docker (Azure#21657)
  update git helper (Azure#21670)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants