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

Implement get_homogeneous_pages and select_homogeneous_pages #90

Open
wants to merge 159 commits into
base: master
Choose a base branch
from

Conversation

mattalbr
Copy link

@mattalbr mattalbr commented Jul 18, 2023

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.

@mattalbr mattalbr marked this pull request as ready for review July 20, 2023 23:12
@mattalbr mattalbr changed the title Experiment with select_homogeneous_pages Implement get_homogeneous_pages and select_homogeneous_pages Jul 20, 2023
@acarapetis
Copy link
Collaborator

AFAIK using select with ORM entities was never officially supported in 1.3, so I think we're good there :)
I'd be perfectly happy with restricting new features to sqlalchemy 1.4+ or even 2+ anyway.

Full review coming soon when I have the time, thanks again :)

@mattalbr
Copy link
Author

mattalbr commented Aug 7, 2023

Just wanted to follow up @acarapetis for a review when you have the chance! Thanks!

@acarapetis
Copy link
Collaborator

@mattalbr Sorry for the delay on this, been a bit swamped. Hopefully will have time this weekend!

@ddorian
Copy link

ddorian commented Sep 12, 2023

Any updates to this?

@mattalbr
Copy link
Author

Hi Anthony, just wanted to ping here to see if you'll have time to review coming up!

@mattalbr
Copy link
Author

Hi @acarapetis, picking this back up, is there any way to get a review from you on this PR?

Copy link
Collaborator

@acarapetis acarapetis left a comment

Choose a reason for hiding this comment

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

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]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

@@ -152,6 +155,7 @@ def prepare_paging(
backwards: bool,
orm: Literal[True],
dialect: Dialect,
page_identifier: Optional[int] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants