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

make PAGE_SIZE configurable for Slack API list calls #612

Merged
merged 7 commits into from
Jan 27, 2021

Conversation

kintarowins
Copy link

Summary

Describe the goal of this PR. Mention any related Issue numbers.

Requirements (place an x in each [ ])

@gitwave gitwave bot added the untriaged label Dec 4, 2020
@CLAassistant
Copy link

CLAassistant commented Dec 4, 2020

CLA assistant check
All committers have signed the CLA.

@@ -43,6 +44,9 @@ class SlackClient
@rtm = new RtmClient options.token, options.rtm
@web = new WebClient options.token, { maxRequestConcurrency: 1 }

unless isNaN(options.apiPageSize)
SlackClient.PAGE_SIZE = parseInt(options.apiPageSize, 10)
Copy link
Member

@seratch seratch Dec 7, 2020

Choose a reason for hiding this comment

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

I know this approach is a pretty easy way to make this work. However, now that the value is no longer a constant, we may want to use an instance filed like @page_size over a class field plus replace the references to SlackClient.PAGE_SIZE in the code.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, yeah I was thinking that too, needed someone to confirm my own doubt. I used @apiPageSize to follow existing camelCase naming convention.

@seratch
Copy link
Member

seratch commented Dec 8, 2020

@kintarowins Thanks a lot for promptly updating the PR. Right now, I'm a bit busy for other tasks but I will be checking if this change works as we expect.

By the way, can you sign our CLA (see the above bot message for details)? 🙇 Without it, we are unable to merge your change.

@kintarowins
Copy link
Author

Signed. I changed default page size to 200 too, what do you think or should it be higher?

@seratch
Copy link
Member

seratch commented Dec 15, 2020

@kintarowins Thanks for signing it. Regarding the page size, I am reluctant to change the default value for keeping backward compatibility. Your change enables developers to customize the size. Let's keep the default as-is.

I did not have time to do some tests today but I'll check this sometime this week.

@seratch seratch added enhancement M-T: A feature request for new functionality semver:minor and removed untriaged labels Dec 17, 2020
@seratch seratch added this to the 4.9.0 milestone Dec 18, 2020
@seratch
Copy link
Member

seratch commented Dec 18, 2020

I just verified if your change works with my Hubot example apps. Before merging this pull request, I will migrate the CI builds to GitHub Actions.

Thanks for your contribution!

@seratch seratch modified the milestones: 4.9.0, 4.8.2 Dec 28, 2020
@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #612 (476fca6) into main (00edee0) will decrease coverage by 0.41%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #612      +/-   ##
==========================================
- Coverage   84.23%   83.82%   -0.42%     
==========================================
  Files           6        6              
  Lines         406      408       +2     
  Branches       90       91       +1     
==========================================
  Hits          342      342              
- Misses         37       38       +1     
- Partials       27       28       +1     
Impacted Files Coverage Δ
src/bot.coffee 73.98% <ø> (ø)
src/client.coffee 89.56% <50.00%> (-1.59%) ⬇️

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 00edee0...476fca6. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality semver:minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants