django-rest-framework icon indicating copy to clipboard operation
django-rest-framework copied to clipboard

Warn about importance of ordering for all pagination classes

Open Elkasitu opened this issue 3 years ago • 10 comments

The ordering recommendations given for the CursorPagination scheme actually apply to all pagination schemes, an unsuspecting developer that implements the more common LimitOffsetPagination or PageNumberPagination classes is unlikely to be aware of the importance of consistent ordering.

This commit moves the Details and limitations section out of the CursorPagination section and puts it as the very first subsection of the Pagination page so that it's one of the first things that developers see.

Some examples of inconsistencies as well as how to deal with them are given, and an extra way to change the ordering of a paginated view is provided.

Fixes #6886

Hopefully this will prevent future developers from scratching their head and wondering why despite a view returning a count of X, going through all the pages yields Y records.

Elkasitu avatar May 20 '22 10:05 Elkasitu

I'm not convinced that this section makes sense when moved around like this.

For example from a first look over - it discusses the ordering attribute on the pagination class. Which is specific to the CursorPagination class, rather than being generally applicable.

lovelydinosaur avatar Jun 06 '22 11:06 lovelydinosaur

You're right, when I wrote this patch I hadn't actually tried to implement CursorPagination yet so I assumed some of these things also applied to the other Pagination classes (like an interface).

I have made some changes, I kept it as a general section because the only Pagination class that is really different from the others is CursorPagination, so having a subsection per class seemed like a lot of duplicated text.

Elkasitu avatar Jun 06 '22 21:06 Elkasitu

Segue sempre sem fazer nenhuma alteração

Faki-140386 avatar Jun 07 '22 01:06 Faki-140386

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Aug 11 '22 22:08 stale[bot]

@tomchristie any chance of ever getting something like this merged? I feel like it's an easy and dangerous trap to fall into

Elkasitu avatar Aug 14 '22 08:08 Elkasitu

@tomchristie any chance of ever getting something like this merged? I feel like it's an easy and dangerous trap to fall into

did you addressed the concerns @tomchristie initially raised?

auvipy avatar Nov 22 '22 06:11 auvipy

@auvipy Well there was only one concern which was about the ordering attribute of the CursorPagination class which I did address, you can see the changes I made in the second commit (can be squashed).

If there are any other concerns I'm willing to address them too

Elkasitu avatar Nov 30 '22 18:11 Elkasitu