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

Paginated response handling #249

Open
jsamoocha opened this issue Nov 9, 2022 · 5 comments
Open

Paginated response handling #249

jsamoocha opened this issue Nov 9, 2022 · 5 comments

Comments

@jsamoocha
Copy link
Collaborator

There are many methods in Client (e.g., get_activities()) that return a BatchedResultsIterator instance. The Strava API parameter that manages how many results are returned per page is per_page (which, BTW, has a deprecation warning in the swagger.json but not in the published API docs).

However, none of the stravalib client methods support the per_page or page parameters that allow paginated results. To better match the Strava API and allow developers to customize paginated responses, wouldn't it be better if the client methods would have these per_page and page parameters as well?

In the short term, a quick-n-dirty workaround is to manually set these attributes on the BatchedResultsIterator instance returned by the client method.

@hozn
Copy link
Collaborator

hozn commented Nov 9, 2022

I agree that this would be an improvement. If that is the only parameter that Strava API supports, I think just adding this to those methods directly makes sense. If it is anticipated that there would be other parameters that would be part of the specification on how results are returned (e.g. other systems might include things like sort field, sort direction, etc.) then probably some sort of PatinationInfo object/dataclass would make sense. (But from memory and this ticket description, that is probably overkill for this API.)

@yihong0618
Copy link
Collaborator

I think we should consider that many users run stravalib in CI
and some of them did not lock the version in requirements.txt
If we have some break change we should be carefully.

@hozn
Copy link
Collaborator

hozn commented Nov 10, 2022

Yeah, I think adding a new optional parameter would be fine (not breaking), but agree that we should be careful about not breaking API if the version is not changing major version numbers.

@lwasser
Copy link
Collaborator

lwasser commented Feb 12, 2024

hi @jsamoocha - this is an older issue but still has some activity. Is this an ongoing issue or did you address this @jsamoocha with the big API restructure? Just lemme know as with the other issues . i can organize things we want to do vs closing older things that have been already addressed as it makes sense!

@lwasser lwasser added the pending-response A label to track issues where we are waiting for a user response label Feb 12, 2024
@jsamoocha jsamoocha removed the pending-response A label to track issues where we are waiting for a user response label Feb 12, 2024
@jsamoocha
Copy link
Collaborator Author

This is not fixed yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants