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

Change websockets imports to make it compatible with older versions #1120

Merged
merged 1 commit into from Sep 21, 2021

Conversation

seratch
Copy link
Member

@seratch seratch commented Sep 21, 2021

Summary

This pull request resolves the build error issue in bolt-python project with python-slack-sdk v3.11.1: https://github.com/slackapi/bolt-python/actions/runs/1255825291

The error is caused by this change: #1117 More specifically, the following import is only compatible with websockets 9.x (the latest major series) while it's not with v8 or older ones.

from websockets.legacy.client import WebSocketClientProtocol

This pull request corrects the import to make it compatible with all the websockets versions. For reference, the reason why bolt-python project detected the error is that the project has some tests for Sanic apps and the Sanic framework still requires websockets v8, not v9.

To prevent similar errors in our code, we may want to run tests with older versions of external dependencies. But at this point, I am thinking that it's not so beneficial while the work like that requires a lot of efforts. I'd like to hold off taking such actions for this purpose.

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 ./scripts/docs.sh?)
  • /docs-src-v2 (Documents, have you run ./scripts/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 self-assigned this Sep 21, 2021
@seratch seratch added this to the 3.11.2 milestone Sep 21, 2021
@seratch seratch added the bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented label Sep 21, 2021
@codecov
Copy link

codecov bot commented Sep 21, 2021

Codecov Report

Merging #1120 (a5c8858) into main (70144f5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1120   +/-   ##
=======================================
  Coverage   88.08%   88.08%           
=======================================
  Files         110      110           
  Lines       10681    10681           
=======================================
  Hits         9408     9408           
  Misses       1273     1273           
Impacted Files Coverage Δ
slack_sdk/socket_mode/websockets/__init__.py 90.18% <100.00%> (ø)

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 70144f5...a5c8858. Read the comment docs.

@seratch seratch merged commit c1c8a3b into slackapi:main Sep 21, 2021
@seratch seratch deleted the websockets-compat branch September 21, 2021 06:03
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 socket-mode Version: 3x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant