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

WIP: Update Send-SlackAPI to support cursor-based pagination #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rkeithhill
Copy link

Fix #88

This is a work in progress because while Get-SlackUser now retrieves all users. I'm not sure what we want to do about the other commands that implement paging themselves. There are also two forms of paging on Slack - an older approach called class paging and a new approach called cursor-based pagination. I've implemented the latter in Send-SlackAPI. Also, with Get-SlackUser I'm not exposing any pagination parameters yet. I think most of the time, folks just want to get all users. That said, we could expose parameters similar to the other commands.

@rkeithhill rkeithhill force-pushed the enable-cursor-based-pagination branch from 68bc48f to 635ff39 Compare June 24, 2019 19:51
@rkeithhill
Copy link
Author

rkeithhill commented Jul 5, 2019

@RamblingCookieMonster Any thoughts on this PR? I think I need to switch the parameter name from EnablePagination to EnablePaging. I could update those other commands to use EnablePaging with an alias of Paging to make this PR not be a breaking change. I'd do something similar with MaxNumberPages with an alias of MaxQueries. So yeah, it would probably be better to update those other commands to remove their paging logic and have them use this new paging logic in Send-SlackAPI.

@RamblingCookieMonster
Copy link
Owner

Agreed on all counts! Alias to avoid breaking change, remove ad hoc paging if you can offload it : D thank you so much for putting the time into this!

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.

Get-Slackuser only getting first 1000 users. Results are paged, can we get all pages please?
3 participants