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

Generalize language for pagination #1642

Merged
merged 4 commits into from Aug 31, 2018

Conversation

2 participants
@turt2live
Member

turt2live commented Aug 31, 2018

Rendered: see 'docs' status check


Previously the section was very strict in what pagination was, however this isn't the reality for the matrix specification. Several endpoints have their own pagination naming conventions and do not follow those mandated by this section.

This commit generalizes the language to cover those endpoints while also describing how pagination works. In particular, it describes the rough API shape to expect and how to deal with the responses.

This commit also removes the M_BAD_PAGINATION error as it is not used in the real world. Homeservers are instead encouraged to use the standard M_INVALID_PARAM or similar error code.

Fixes #610
Fixes #1523

Generalize language for pagination
Previously the section was very strict in what pagination was, however this isn't the reality for the matrix specification. Several endpoints have their own pagination naming conventions and do not follow those mandated by this section.

This commit generalizes the language to cover those endpoints while also describing how pagination works. In particular, it describes the rough API shape to expect and how to deal with the responses.

This commit also removes the `M_BAD_PAGINATION` error as it is not used in the real world. Homeservers are instead encouraged to use the standard `M_INVALID_PARAM` or similar error code.

Fixes #610
Fixes #1523

@turt2live turt2live requested a review from matrix-org/spec-core-team Aug 31, 2018

@turt2live turt2live added this to In review (just the PRs) in August 2018 r0 via automation Aug 31, 2018

@uhoreg

I don't know if I have enough context to review the correctness of this (tough it seems right), but here are some wording suggestions.

of what is being paginated, there is a common approach which is used to give
clients an easy way of selecting subsets of a potentially changing dataset. Each
endpoint that uses pagination may use different parameters, however the theme
amoung them is that they take a ``from`` and ``to`` token, and occasionally

This comment has been minimized.

@uhoreg

uhoreg Aug 31, 2018

Member

amoung

``to=END``, without a limit set.
of what is being paginated, there is a common approach which is used to give
clients an easy way of selecting subsets of a potentially changing dataset. Each
endpoint that uses pagination may use different parameters, however the theme

This comment has been minimized.

@uhoreg

uhoreg Aug 31, 2018

Member

fullstop between "parameters" and 'however"

clients an easy way of selecting subsets of a potentially changing dataset. Each
endpoint that uses pagination may use different parameters, however the theme
amoung them is that they take a ``from`` and ``to`` token, and occasionally
a ``limit`` and ``dir`` to describe the direction to look in. Together, these

This comment has been minimized.

@uhoreg

uhoreg Aug 31, 2018

Member

This kind of sounds like the limit and dir parameters together describe the direction to look in. Maybe put dir first (so that it's "a dir, to describe the direction to look in, and a limit"), or just drop the "to describe the direction to look in", since it's described below.

@turt2live turt2live requested a review from matrix-org/spec-core-team Aug 31, 2018

@uhoreg

uhoreg approved these changes Aug 31, 2018

Looked over some endpoints that do pagination, and it looks fairly accurate, asside from a suggested sentence change.

direction of events to return: either forwards (``f``) or backwards (``b``).
The response contains a ``start`` or ``prev_batch`` token which references the
result set immediately prior to the returned set. The response might additionally
have an ``end`` or ``next_batch`` token to indicate the results after the returned

This comment has been minimized.

@uhoreg

uhoreg Aug 31, 2018

Member

/notifications uses next_token. /search uses next_batch and not prev_batch. (Ugh) So maybe these two sentences might be better as "The response may contain tokens that can be used for retrieving results before or after the returned set. These tokens may be called start or prev_batch for retrieving the previous result set, or end, next_batch or next_token for retrieving the next result set."

This comment has been minimized.

@turt2live

turt2live Aug 31, 2018

Member

(will generalize some more)

August 2018 r0 automation moved this from In review (just the PRs) to Reviewer approved Aug 31, 2018

@turt2live

This comment has been minimized.

Member

turt2live commented Aug 31, 2018

I've taken your suggestion verbatim as it seems great to me.

@turt2live turt2live merged commit dfe6d0d into matrix-org:master Aug 31, 2018

7 checks passed

ci/circleci: build-dev-scripts Your tests passed on CircleCI!
Details
ci/circleci: build-docs Your tests passed on CircleCI!
Details
ci/circleci: build-swagger Your tests passed on CircleCI!
Details
ci/circleci: check-docs Your tests passed on CircleCI!
Details
ci/circleci: validate-docs Your tests passed on CircleCI!
Details
docs Click details to preview the HTML documentation.
Details
swagger Click to preview the swagger build.
Details

August 2018 r0 automation moved this from Reviewer approved to Done (this list will be incomplete) Aug 31, 2018

@turt2live turt2live deleted the turt2live:travis/c2s/pagination branch Aug 31, 2018

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