Skip to content

Conversation

@RodneyU215
Copy link
Contributor

Summary

When using RTMClient#typing(), the typing indicator was not being sent immediately because the function was not asynchronous. This caused the erroneous behavior described in #415. In order to immediately send any data over the websocket with aiohttp we must use an async function.

This PR changes both #typing() and #ping() functions on the RTMClient class into async functions.

Here's an example of what this would look like implemented:

@slack.RTMClient.run_on(event="open")
async def typing_message(**payload):
    rtm_client = payload["rtm_client"]
    await rtm_client.typing(channel="C01234567")

It's a minor but still backwards-incompatible change.

Requirements

@codecov
Copy link

codecov bot commented Jun 18, 2019

Codecov Report

Merging #446 into master will increase coverage by 0.07%.
The diff coverage is 72.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
+ Coverage   56.94%   57.02%   +0.07%     
==========================================
  Files           6        6              
  Lines         734      719      -15     
  Branches       42       39       -3     
==========================================
- Hits          418      410       -8     
+ Misses        307      301       -6     
+ Partials        9        8       -1
Impacted Files Coverage Δ
slack/web/client.py 33.72% <0%> (+0.07%) ⬆️
slack/web/base_client.py 85.71% <42.85%> (+1.29%) ⬆️
slack/rtm/client.py 92.3% <94.73%> (+2.3%) ⬆️

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 cedbadc...ebd4ddc. Read the comment docs.

@RodneyU215 RodneyU215 added Priority: High bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented Version: 2x labels Jun 20, 2019
@RodneyU215 RodneyU215 self-assigned this Jun 20, 2019
@lavabyrd
Copy link

This looks good to me. Its definitely unfortunate this was missed the first time around but it looks good

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: 2x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants