Skip to content

Fix Web Client custom iterator#521

Merged
RodneyU215 merged 4 commits intoslackapi:masterfrom
smaeda-ks:smaeda-ks/fix-iterator
Oct 5, 2019
Merged

Fix Web Client custom iterator#521
RodneyU215 merged 4 commits intoslackapi:masterfrom
smaeda-ks:smaeda-ks/fix-iterator

Conversation

@smaeda-ks
Copy link
Contributor

@smaeda-ks smaeda-ks commented Sep 28, 2019

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 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Sep 28, 2019 •
edited
Loading

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.

@smaeda-ks smaeda-ks force-pushed the smaeda-ks/fix-iterator branch from 990f64d to 2997181 Compare September 28, 2019 16:37
@seratch
Copy link
Contributor

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 •
edited
Loading

@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

smaeda-ks commented Oct 3, 2019

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
Contributor

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
Contributor

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

smaeda-ks commented Oct 3, 2019

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

seratch
seratch approved these changes Oct 3, 2019
View reviewed changes
Copy link
Contributor

@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?

@seratch seratch requested a review from RodneyU215 October 3, 2019 13:59
@RodneyU215
Copy link
Contributor

RodneyU215 commented Oct 5, 2019

@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
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RodneyU215 RodneyU215 Awaiting requested review from RodneyU215

1 more reviewer

@seratch seratch seratch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@smaeda-ks @CLAassistant @seratch @RodneyU215