Skip to content
This repository was archived by the owner on Nov 15, 2024. It is now read-only.

Conversation

rchodava
Copy link
Contributor

No description provided.

@rchodava rchodava requested a review from codetheweb as a code owner July 22, 2023 01:21
@rchodava rchodava requested review from seveibar and Andrewjeska July 22, 2023 01:21
Copy link
Contributor

@seveibar seveibar left a comment

Choose a reason for hiding this comment

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

Note that the pagination object isnt exposed, so the sdk has to internally implement pagination a consumer cant/shouldnt implement, but i think providing these parameters should be allowed

@rchodava
Copy link
Contributor Author

Note that the pagination object isnt exposed, so the sdk has to internally implement pagination a consumer cant/shouldnt implement, but i think providing these parameters should be allowed

yeah, I liked the idea of a separate listAll which does the pagination within the client, with the list sort of just a thin wrapper on the HTTP API method - provides the parameters the API method accepts

Slight chance that if anyone is expecting list to already return all devices, this will be breaking. But I think no one is atm

@seveibar
Copy link
Contributor

seveibar commented Jul 22, 2023

yeah, I liked the idea of a separate listAll which does the pagination within the client, with the list sort of just a thin wrapper on the HTTP API method - provides the parameters the API method accepts

Hmm I'd prefer to just have pagination be automatically implemented. Why would we ever make someone implement pagination? That said these parameters aren't strictly related to pagination so I think it's totally fine to allow them- but I would always automatically paginate for the user unless the page size was excessive (in which case I think we should throw a special error)

@rchodava
Copy link
Contributor Author

yeah, I liked the idea of a separate listAll which does the pagination within the client, with the list sort of just a thin wrapper on the HTTP API method - provides the parameters the API method accepts

Hmm I'd prefer to just have pagination be automatically implemented. Why would we ever make someone implement pagination? That said these parameters aren't strictly related to pagination so I think it's totally fine to allow them- but I would always automatically paginate for the user unless the page size was excessive (in which case I think we should throw a special error)

Do you mean a helper paginator object that's returned as a result from list-like methods? Yeah, that makes sense. And yeah, these parameters are not going to get in the way of that.

@rchodava rchodava merged commit ef99e62 into main Jul 22, 2023
seambot pushed a commit that referenced this pull request Jul 22, 2023
## [7.3.3](seamapi/javascript@v7.3.2...v7.3.3) (2023-07-22)

### Bug Fixes

* Add pagination params to devices list ([#247](seamapi/javascript#247)) ([ef99e62](seamapi/javascript@ef99e62))
@seambot
Copy link
Contributor

seambot commented Jul 22, 2023

🎉 This PR is included in version 7.3.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@seveibar
Copy link
Contributor

Do you mean a helper paginator object that's returned as a result from list-like methods? Yeah, that makes sense. And yeah, these parameters are not going to get in the way of that.

list-like methods should just always paginate for the full result set, otherwise access_codes.list couldn't return just a list of access codes- it would need to return a special object or we would have to introduce another method etc. the point of the SDK is to make interfacing the API really easy so I think pagination is a nice thing to not have to worry about unless you're doing http directly

@seveibar seveibar deleted the devices_list_pagination_params branch July 22, 2023 03:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants