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

SocketModeClient expose async event loop param #1609

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

jantman
Copy link
Contributor

@jantman jantman commented Dec 4, 2024

Summary

This PR exposes the loop (asyncio event loop) parameter of the async SocketModeClient, so that the socket mode client can be used inside an existing asyncio event loop.

Testing

Ensure that the parameter is actually passed through? This is a rather trivial change just to expose a parameter that currently isn't.

Category

  • 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 (Documents)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements

  • 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.

Copy link

salesforce-cla bot commented Dec 4, 2024

Thanks for the contribution! Before we can merge this, we need @jantman to sign the Salesforce Inc. Contributor License Agreement.

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.93%. Comparing base (cc070d3) to head (1dc6471).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1609   +/-   ##
=======================================
  Coverage   84.93%   84.93%           
=======================================
  Files         113      113           
  Lines       12629    12630    +1     
=======================================
+ Hits        10726    10727    +1     
  Misses       1903     1903           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hello-ashleyintech
Copy link
Contributor

@jantman looks like although the CI checks passed, 3.11 and 3.12 are returning annotations of runtime testing errors: RuntimeError: Event loop is closed. Could you take a look into what might be causing this, or this an expected byproduct of your PR? If so, what can we do to eliminate this error since it seems to be introduced by the PR?

@seratch
Copy link
Member

seratch commented Dec 5, 2024

@hello-ashleyintech Thanks for checking this. It seems the warning/error logs can occur without this PR's changes. So, the unit tests might need to be improved, but there is no need to check its details this time.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks for sending this PR! While there are a few things to tweak such as docstring comment updates and adding unit tests, the change looks good to me. I will update the missing ones after merging this PR.

@seratch seratch added this to the 3.33.5 milestone Dec 5, 2024
@seratch seratch merged commit 8b8085e into slackapi:main Dec 5, 2024
12 checks passed
seratch added a commit that referenced this pull request Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants