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

Clarify page number for pagination #187

Merged
merged 5 commits into from
Nov 6, 2019
Merged

Clarify page number for pagination #187

merged 5 commits into from
Nov 6, 2019

Conversation

merkys
Copy link
Member

@merkys merkys commented Oct 3, 2019

The changes:

  1. Using page_number throughout the document (was page_page in one occurrence);
  2. Making page_number explicitly 1-based.

Fixes #186

@merkys merkys added the PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing label Oct 3, 2019
@rartino rartino added this to the 1.0 release milestone Oct 25, 2019
CasperWA
CasperWA previously approved these changes Oct 31, 2019
Copy link
Member

@CasperWA CasperWA left a 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?

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@rartino rartino added this to In progress in Road to OPTIMADE 1.0 Nov 3, 2019
Co-Authored-By: Rickard Armiento <gitcommits@armiento.net>
@merkys
Copy link
Member Author

merkys commented Nov 5, 2019

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?

Here "size" stands for the number of items on the page, fixed that in 837e83e.

Is it possible to add a reference to pagination definitions somewhere on www? Or is it considered bad practice?

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.

@CasperWA
Copy link
Member

CasperWA commented Nov 5, 2019

Here "size" stands for the number of items on the page, fixed that in 837e83e.

Sure - but doesn't it always? I.e., shouldn't this explaining parenthesis be placed earlier?

Is it possible to add a reference to pagination definitions somewhere on www? Or is it considered bad practice?

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.

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.

@merkys
Copy link
Member Author

merkys commented Nov 5, 2019

Sure - but doesn't it always? I.e., shouldn't this explaining parenthesis be placed earlier?

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 page_limit?

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.

Again, maybe @dwinston knows a suitable reference?

@merkys merkys requested a review from dwinston November 5, 2019 11:49
Copy link
Contributor

@rartino rartino left a 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.

optimade.rst Outdated Show resolved Hide resolved
Co-Authored-By: Rickard Armiento <gitcommits@armiento.net>
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Perfect, thanks @merkys!

Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

All good.

@rartino rartino merged commit d72d453 into Materials-Consortia:develop Nov 6, 2019
Road to OPTIMADE 1.0 automation moved this from In progress to Done Nov 6, 2019
@merkys merkys deleted the clarify-page-number branch November 6, 2019 12:05
@merkys
Copy link
Member Author

merkys commented Nov 6, 2019

Thanks for the reviews!

CasperWA added a commit to CasperWA/optimade-python-tools that referenced this pull request Feb 6, 2020
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.
CasperWA added a commit to CasperWA/optimade-python-tools that referenced this pull request Feb 6, 2020
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Conflicting query parameters 'page_page' and 'page_number'
3 participants