Implement get_homogeneous_pages and select_homogeneous_pages#90
Implement get_homogeneous_pages and select_homogeneous_pages#90mattalbr wants to merge 159 commits into
Conversation
Unit tests forthcoming once we're directionally aligned.
mattalbr
changed the title
|
AFAIK using Full review coming soon when I have the time, thanks again :) |
|
Just wanted to follow up @acarapetis for a review when you have the chance! Thanks! |
|
@mattalbr Sorry for the delay on this, been a bit swamped. Hopefully will have time this weekend! |
|
Any updates to this? |
|
Hi Anthony, just wanted to ping here to see if you'll have time to review coming up! |
|
Hi @acarapetis, picking this back up, is there any way to get a review from you on this PR? |
acarapetis
left a comment
acarapetis
left a comment
There was a problem hiding this comment.
Took me a long while to get to this - hopefully it's still useful to you to get this merged.
Looks good overall - I've left a couple of minor comments.
| unpaged: list | ||
| gathered: deque | ||
| backwards: bool | ||
| page: Tuple[Union[MarkerLike, str], bool] |
There was a problem hiding this comment.
This type is confusing me and making pyright raise a bunch of errors in check_multiple_paging_*. I think it should just be page: MarkerLike?
| backwards: bool, | ||
| orm: Literal[True], | ||
| dialect: Dialect, | ||
| page_identifier: Optional[int] = None, |
There was a problem hiding this comment.
There are page_identifier: Optional[int] parameters on a few functions in this module, but none of them seem to be used. Am I missing something, or they can be removed?
3 participants
The select version was super challenging to write, but we made it. Lots of type wrangling to get the query to work with orm entities. Doesn't work in 1.3, but neither does select(Book, Book.id) with a normal page. Figure it's probably pretty rare to be using select() and 1.3 anyway.