-
Notifications
You must be signed in to change notification settings - Fork 37
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
Clarify page number for pagination #187
Clarify page number for pagination #187
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All fine for me - thanks @merkys.
Only a single comment, since I am unsure of all the different pagination setups. I am guessing that for the "page-based" pagination, page "size" is a specific thing?
Is it possible to add a reference to pagination definitions somewhere on www? Or is it considered bad practice?
Co-Authored-By: Rickard Armiento <gitcommits@armiento.net>
Here "size" stands for the number of items on the page, fixed that in 837e83e.
I am not aware of any RFC or like document describing the pagination strategies. These seem straightforward (IMO), but I fully understand the need for the reference here. |
Sure - but doesn't it always? I.e., shouldn't this explaining parenthesis be placed earlier?
Fair enough - it was also only if we knew of something, since it seemed to me the different kinds of pagination here were listed as if obvious and well-defined at another source. |
It should, and it seems that it is explained earlier. I suppose we can remove it. It seems @dwinston has touched this text recently. @dwinston maybe you remember why we have this additional clarification of
Again, maybe @dwinston knows a suitable reference? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @CasperWA that the clarification of page_limit
is superfluous here. The use of it to set the sizes of the pages that are paginated should be obvious given its use in all pagination modes, and the general description just above.
Co-Authored-By: Rickard Armiento <gitcommits@armiento.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks @merkys!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good.
Thanks for the reviews! |
Remove PEP8 E251/E252 (unexpected spaces around keyword / parameter equals) from lint warnings. Update `page_page` to `page_number` according to Materials-Consortia/OPTIMADE#187. Due to pydantic: https://pydantic-docs.helpmanual.io/usage/schema/#unenforced-field-constraints the `ge` parameter cannot be used together with a non-standard type, e.g. NonnegativeInt. Instead the OpenAPI parameter is set directly, here `minimum`. Regular expressions are added for `response_fields` and `sort` based on the regular expressions provided for an `identifier` in the OPTiMaDe spec.
Remove PEP8 E251/E252 (unexpected spaces around keyword / parameter equals) from lint warnings. Update `page_page` to `page_number` according to Materials-Consortia/OPTIMADE#187. Due to pydantic: https://pydantic-docs.helpmanual.io/usage/schema/#unenforced-field-constraints the `ge` parameter cannot be used together with a non-standard type, e.g. NonnegativeInt. Instead the OpenAPI parameter is set directly, here `minimum`. Regular expressions are added for `response_fields` and `sort` based on the regular expressions provided for an `identifier` in the OPTiMaDe spec.
The changes:
page_number
throughout the document (waspage_page
in one occurrence);page_number
explicitly 1-based.Fixes #186