-
Notifications
You must be signed in to change notification settings - Fork 849
Fix #1100 by improving the get/__getitem__ method behavior when response body is empty #1103
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
Fix #1100 by improving the get/__getitem__ method behavior when response body is empty #1103
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1103 +/- ##
==========================================
- Coverage 86.19% 86.18% -0.02%
==========================================
Files 110 110
Lines 9946 9951 +5
==========================================
+ Hits 8573 8576 +3
- Misses 1373 1375 +2
Continue to review full report at Codecov.
|
…en response body is empty
73ccf12 to
fb44371
Compare
|
|
||
| def __str__(self): | ||
| """Return the Response data if object is converted to a string.""" | ||
| if isinstance(self.data, bytes): |
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.
While SlackResponse has this validation, the same thing has been missing in slack.web.AsyncSlackResponse (legacy one).
| raise ValueError( | ||
| "As the response.data is binary data, this operation is unsupported" | ||
| ) | ||
| if self.data is None: |
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.
This is added for resolving the issue #1100
| raise ValueError( | ||
| "As the response.data is binary data, this operation is unsupported" | ||
| ) | ||
| if self.data is None: |
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.
get method should return None even if the data is None in the same manner with dictionary objects.
| Raises: | ||
| SlackApiError: The request to the Slack API failed. | ||
| """ | ||
| if self._logger.level <= logging.DEBUG: |
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.
This should have been removed in #1094
| self._iteration = None # for __iter__ & __next__ | ||
| self._client = client | ||
| self._logger = logging.getLogger(__name__) | ||
|
|
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.
For backward compatibility, we have two AsynSlackResponse under slack and slack_sdk packages. We have to apply the same changes to the two this time.
Summary
This pull request fixes #1100 by improving
SlackResponseandAsyncSlackResponse.Category (place an
xin each of the[ ])/docs-src(Documents, have you run./docs.sh?)/docs-src-v2(Documents, have you run./docs-v2.sh?)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements (place an
xin each[ ])python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.