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

Allow limit and cursor on calls with pagination. General cleanup of u… #231

Closed
wants to merge 1 commit into from
Closed

Conversation

alexherreid
Copy link

Adds the ability to request number of return groups/users/channels in a given request, plus supports the cursor to get additional pages. Assorted string formatting and file cleanup.

@alexherreid
Copy link
Author

#230 would be resolved by this

@Inumedia
Copy link
Owner

Inumedia commented Apr 1, 2020

While I'm sure this does fix the pagination issue, this also applies a lot of styling changes across the board. I can't accept it as is.

@alexherreid
Copy link
Author

Thanks for the reply. Could you help me better understand why the additional cleanup/styling is a bad thing/undesired? All tests still pass, it adds the missing feature, and those changes help provide cleanup and correct some coding conventions along the way.

@gpailler
Copy link
Collaborator

gpailler commented Apr 1, 2020

I guess @Inumedia expects 2 PR for this kind of change

  • One with the fix for the pagination
  • One for the code styling changes

It's better to have a specific purpose per PR (easier to track and manage).
Usually, the idea is to adapt the PR to the existing code style (even if the styling is not perfect in the SlackAPI codebase 😄 ) and discuss with the maintainer before a global styling change.

@Inumedia
Copy link
Owner

Inumedia commented Apr 1, 2020

Ah, sorry for the confusion!

Yeah, it's exactly as @gpailler says. I'm all for improving code quality and conventions, and improvements in general, but this PR has a lot of changes and causes conflicts with other PRs I merged earlier.

If you need pagination implemented asap, I suggest creating a pull request with just those required changes.

As for the conventions, those should be in their own PR and maybe some discussion around them.

@Inumedia Inumedia closed this Aug 18, 2020
@Inumedia
Copy link
Owner

Closed due to inactivity from PR author, please submit two new PRs one for style and one for pagination

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.

3 participants