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

bugfix that pagination sets upper limit when should not! #70

Merged
merged 2 commits into from
Feb 5, 2023

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Feb 4, 2023

@wolfv see the changes here - I removed the typing because it was really just hurting us more than helping!

Signed-off-by: vsoch [email protected]

@vsoch vsoch mentioned this pull request Feb 4, 2023
@wolfv
Copy link
Contributor

wolfv commented Feb 5, 2023

Hmm, I thought about pagination more and I think the n is actually OK because it just indicates how many to get per "page" (not in total). So maybe better to keep an arbitrarily high value to not use too many requests, but I haven't checked wether it makes a difference or not.

@vsoch
Copy link
Contributor Author

vsoch commented Feb 5, 2023

Hmm, I thought about pagination more and I think the n is actually OK because it just indicates how many to get per "page" (not in total). So maybe better to keep an arbitrarily high value to not use too many requests, but I haven't checked wether it makes a difference or not.

I think N is typically how many results to get - it would be strange to only get a subset per page (but then to paginate!)

Signed-off-by: vsoch <[email protected]>
@vsoch
Copy link
Contributor Author

vsoch commented Feb 5, 2023

I updated the PR here to also include handling the case when tags is null. Let me know if we are ok to merge and release!

@wolfv
Copy link
Contributor

wolfv commented Feb 5, 2023

Yeah but 'n' would also be used to set the elements per page. But it also doesn't matter much to me as ping as it works :)

@wolfv
Copy link
Contributor

wolfv commented Feb 5, 2023

All good regarding merging and releasing btw!

@vsoch
Copy link
Contributor Author

vsoch commented Feb 5, 2023

It would be confusing for n to set elements per page, but have the endpoint be different (the total):

image

I've implemented / used a lot of APIs and (I think)? the parameter you have in mind is usually called per_page or similar.

Let's get this out!

@vsoch vsoch merged commit 62cf1e3 into main Feb 5, 2023
@vsoch vsoch deleted the fix/tags-all-bug branch February 5, 2023 19:17
@vsoch vsoch mentioned this pull request Feb 5, 2023
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.

2 participants