search

photocrowd / django-cursor-pagination

Cursor-based pagination for Django
BSD 3-Clause "New" or "Revised" License
156 stars 27 forks source link

Implement NULL value ordering #45

Closed untidy-hair closed 1 year ago

untidy-hair commented 1 year ago

Solves #44

On a second thought, maybe it's easier to be able to see diffs than just the whole Gist file so I'm creating this PR as a draft for discussions 🙏

TODO:

[EDIT] Just for convenience, I'm copying some contents from #44

👋 First of all, thank you for this awesome library! I encountered this None/Null issue at my workplace (modified) where the last object of the previous page has NULL value for one of the ordering keys.

ERROR: invalid input syntax for type smallint: "None" at character 1564

... AND ("product"."tier" > 'None' OR ("product"."tier" = 'None' AND "product"."id" > '555'))) ORDER BY "product"."tier" ASC, "product"."id" DESC LIMIT 101

I made a patch and that is working for me but now I am trying to update this library, so I can use it as a regular library.

Whether Null comes at the beginning or at the end of the order is DB dependent. In that sense, my implementation there is a little bit opinionated for always putting NULL at the end (except when "last" exists). I personally cannot think of any reasons why NULL should come first but that can be configurable, too, if we like. (Caveat on README should be updated with this)

untidy-hair commented 1 year ago

Additional work is going on to cover cases I can think of.

thommor commented 1 year ago

Thanks for the PR, if you let me know when you think it's ready I'll do a full review.

untidy-hair commented 1 year ago

Hi @thommor , thanks for the offering! I think this PR is ready for review.

untidy-hair commented 1 year ago

I've just left one comment, but overall I like this addition.

I need to go through and overhaul this project like setting up releases into PyPi etc, which I will plan to do next week. Happy to merge this before then but I probably won't push a new version to pypi until I've done my side.

Thanks for the review! Updated the var name per suggestion. The plan of the new version sounds good to me 👍