[Bug Fix] Return error message when body has no length when logging e… #1082
Conversation
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| def _redacted_dump(self, prefix: str, body: str) -> str: | ||
| if len(body) == 0: | ||
| return "" | ||
| try: |
There was a problem hiding this comment.
I think we should check isinstance(body, str) instead of try catch
There was a problem hiding this comment.
right, I think isinstance would be better.
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
tanmay-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| if not isinstance(body, str): | ||
| return "unsupported body type" |
There was a problem hiding this comment.
The method signature says that this should be a string. Either adjust the type to cover all potential cases or make sure that the method is only called with string inputs.
|
|
||
| ### Bug Fixes | ||
| - Improving the error message that is shown when the unsupported `dbutils.credentials.getServiceCredentialsProvider` method is used. This method can only be used inside of a notebook. | ||
| - Check for string body type while logging error message. |
There was a problem hiding this comment.
I think we could have a message that explains a little more what the change is about. For example, as a reader, I'm wondering "what does checking for string body type" actually does? I believe something like the following would more useful:
Fix a
TypeErrorerror when logging non-string body types inRoundTrip.
Also, could you update the change log above so that it phrased at the imperative tense? That is "Improve the error message ..."
There was a problem hiding this comment.
Hi @renaudhartert-db, thanks for the review.
The PR is WIP and I just added a short line so it doesn't fail check for next changelog test, was planning to elaborate it and add test before sending it to review.
|
closing this since @serena-ruan has a fix for this issue. |
WIP
What changes are proposed in this pull request?
When logging error, we check for empty body by using len(), however this doesn't work for example with
_io.BytesIOTypeError: object of type '_io.BytesIO' has no len()How is this tested?