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

show detailed error #18229

Merged
merged 5 commits into from
May 4, 2021
Merged

show detailed error #18229

merged 5 commits into from
May 4, 2021

Conversation

xiangyan99
Copy link
Member

No description provided.

@xiangyan99 xiangyan99 requested a review from lmazuel as a code owner April 22, 2021 18:00
@xiangyan99 xiangyan99 requested a review from iscai-msft April 22, 2021 21:18
@xiangyan99
Copy link
Member Author

#14909

@xiangyan99 xiangyan99 requested a review from annatisch May 3, 2021 16:09
(schema https://github.com/OAI/OpenAPI-Specification/blob/master/schemas/v2.0/schema.json)."
}
]
}
Copy link
Member

Choose a reason for hiding this comment

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

Would be worth checking how this renders in sphinx (if it's actually in the public docs).
It may need additional code-block tags for the formatting to work.

Copy link
Member Author

Choose a reason for hiding this comment

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

From my experiment, .. code-block:: tag will make the json invisible in sphinx

@@ -160,13 +181,14 @@ def error(self):
return self

def __str__(self):
return "({}) {}".format(self.code, self.message)
return self.message_details()
Copy link
Member

Choose a reason for hiding this comment

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

Is str() also supposed to have a character limit? Or is that just repr()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't find there is a character limit in str()?

Copy link
Member

Choose a reason for hiding this comment

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

It's in our guidelines - but looks to only be specified for repr:
https://azure.github.io/azure-sdk/python_implementation.html#python-models-repr-length

So str might be okay?

sdk/core/azure-core/azure/core/exceptions.py Outdated Show resolved Hide resolved
@xiangyan99
Copy link
Member Author

Technically, yes. But given the issue is to update exception error message. I think it is acceptable?

@annatisch
Copy link
Member

annatisch commented May 4, 2021

You could refactor it like this to make it otherwise unchanged:

def __str__(self):
        return "({}) {}\n{}".format(
            self.code,
            self.message
            self.message_details()
        )

def message_details(self):
    """
    Original message_details method unchanged
    """
    error_str = "Code: {}".format(self.code)
    error_str += "\nMessage: {}".format(self.message)
    ...

@xiangyan99
Copy link
Member Author

I thought about it :). But I felt a little weird if we show slightly different messages between str() & message_details(). I was concerned It would cause confusions.

I will update my PR.

@xiangyan99 xiangyan99 requested a review from annatisch May 4, 2021 19:39
@xiangyan99 xiangyan99 merged commit 9686966 into master May 4, 2021
@xiangyan99 xiangyan99 deleted the core_detailed_error_message branch May 4, 2021 20:41
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request May 5, 2021
…into azure_purview_catalog

* 'master' of https://github.com/Azure/azure-sdk-for-python: (109 commits)
  [Tables] Adds support for AzureNamedKeyCredential (Azure#18456)
  [Tables] delete_entity takes an entity instead of row and partition key (Azure#18269)
  [Tables] Removed TableEntity attribute wrapper (Azure#18489)
  [EventHub&ServiceBus] Bump uAMQP dependency (Azure#17942)
  [ServiceBus] add keyword override support to update_ methods in mgmt module (Azure#18210)
  Add compatibility switch to disable CAE (Azure#18148)
  Service Bus Named Key Credential (Azure#18471)
  Change to use dynamic resource connection string for chat tests and identity samples (Azure#18502)
  Increase dependency (Azure#18500)
  show detailed error (Azure#18229)
  prerelease (Azure#18507)
  [Container Registry] addressing issues (Azure#18486)
  update per_call_policies & per_retry_policies (Azure#18406)
  Eh named key (Azure#18292)
  [Tables] Updating EntityProperty (Azure#18177)
  [Service Bus] fix async auth test (Azure#18499)
  [communication] Live Testing - Introduce CloudConfig into test.yml  (Azure#18469)
  Release azure-servicefabric 8.0 (Azure#18488)
  [Communication]: Updated test_search_available_phone_numbers_with_invalid_country_code async test to be consistent with sync test (Azure#18466)
  [Container Registry] DeleteRepositoryResult Changed (Azure#18443)
  ...
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Mar 17, 2022
update readme (Azure#18229)

* update readme

* Update readme.python.md

* Update readme.python.md

* Update readme.md

Co-authored-by: Jiefeng Chen <51037443+BigCat20196@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants