Skip to content

Conversation

@seratch
Copy link
Contributor

@seratch seratch commented Aug 20, 2021

Summary

This pull request fixes #1100 by improving SlackResponse and AsyncSlackResponse.

Category (place an x in each of the [ ])

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /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 x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

@seratch seratch added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented web-client Version: 3x labels Aug 20, 2021
@seratch seratch added this to the 3.10.0 milestone Aug 20, 2021
@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #1103 (61f0f9d) into main (86faa33) will decrease coverage by 0.01%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
slack_sdk/web/async_slack_response.py 86.88% <50.00%> (-3.12%) ⬇️
slack_sdk/web/slack_response.py 93.33% <100.00%> (+0.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86faa33...61f0f9d. Read the comment docs.

@seratch seratch force-pushed the issue-1100-empty-response-body branch from 73ccf12 to fb44371 Compare August 20, 2021 00:14

def __str__(self):
"""Return the Response data if object is converted to a string."""
if isinstance(self.data, bytes):
Copy link
Contributor Author

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:
Copy link
Contributor Author

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:
Copy link
Contributor Author

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:
Copy link
Contributor Author

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__)

Copy link
Contributor Author

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.

@seratch seratch merged commit c23312c into slackapi:main Aug 20, 2021
@seratch seratch deleted the issue-1100-empty-response-body branch August 20, 2021 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 3x web-client

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unhandled exception: The server responded with: None

1 participant