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

Fix Web Client custom iterator #521

Merged
merged 4 commits into from
Oct 5, 2019

Conversation

smaeda-ks
Copy link
Contributor

Summary

For Web Client, since SlackResponse class overrides an initial response data when iterating through the response object, it only returns the last fetched response data when it's iterated twice.

import slack

client = slack.WebClient(token='redacted')
response = client.channels_list(limit=50)

print('#### 1st iteration ####')

for i in response:
    print(i['channels'][0]['id'])

print('#### 2nd iteration ####')

for i in response:
    print(i['channels'][0]['id'])

Actual result

#### 1st iteration ####
C9A81JQQ4
CB2LQDJTB
CB34D9SQ3
CB3BF4953
CB48SJH1V
#### 2nd iteration ####
CB48SJH1V

Expected result

#### 1st iteration ####
C9A81JQQ4
CB2LQDJTB
CB34D9SQ3
CB3BF4953
CB48SJH1V
#### 2nd iteration ####
C9A81JQQ4
CB2LQDJTB
CB34D9SQ3
CB3BF4953
CB48SJH1V

Requirements (place an x in each [ ])

@CLAassistant
Copy link

CLAassistant commented Sep 28, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Sep 28, 2019

Codecov Report

Merging #521 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #521      +/-   ##
==========================================
+ Coverage   68.89%   68.92%   +0.03%     
==========================================
  Files          15       15              
  Lines        1707     1709       +2     
  Branches       96       96              
==========================================
+ Hits         1176     1178       +2     
  Misses        508      508              
  Partials       23       23
Impacted Files Coverage Δ
slack/web/slack_response.py 97.77% <100%> (+0.1%) ⬆️

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 128305a...6a3bcec. Read the comment docs.

@seratch
Copy link
Member

seratch commented Oct 3, 2019

I have tried to reproduce your situation but I can't.

@smaeda-ks
Copy link
Contributor Author

smaeda-ks commented Oct 3, 2019

@seratch Thanks, how did you test it? FYI, I tested it on my private workspace where there are over 200+ channels (and that's why I set limit=50 sorry if that was not obvious for you).

$ pip list | grep slackclient
slackclient   2.2.0

$ python --version
Python 3.7.3

@smaeda-ks
Copy link
Contributor Author

Let me know if you need more clarification here. I'm pretty sure it's reproducible as long as you set the limit parameter value less than the total number of channels you're testing on.

@seratch
Copy link
Member

seratch commented Oct 3, 2019

set the limit parameter value less than the total number of channels

Thanks, I managed to reproduce it.

@seratch
Copy link
Member

seratch commented Oct 3, 2019

I confirmed that your fix addresses the issue by running it towards actual Slack APIs.

I wrote a unit test for this PR. Would you consider merging smaeda-ks#1 (or feel free to modify it as you like)? Then, I'll give 👍 to this PR.

@smaeda-ks
Copy link
Contributor Author

@seratch Thanks for your help! Merged the PR to my forked branch so feel free to merge this PR accordingly.

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.

@RodneyU215 I believe this one is ready to merge. Could you review this change when you have time?

@RodneyU215
Copy link
Contributor

@smaeda-ks Thank you for the PR and thank you @seratch for adding in some tests. Super helpful fix. Looks good!

@RodneyU215 RodneyU215 merged commit 70883cb into slackapi:master Oct 5, 2019
@RodneyU215 RodneyU215 mentioned this pull request Oct 8, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants