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

refactor: remove offset/limit where too generic #624

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Conversation

nathanielc
Copy link
Collaborator

This change removes the offset/limit pagination pattern where it implies a more flexible API than was needed or used. Having the flexible API meant that casts from usize to smaller types for limits and offsets could break pagination assumptions. Now with this change more specific APIs are exposed where its safe to make assumptions about the size of limit offset values in queries.

Additionally in the API where true pagination was the desired API u32 is used instead of usize to keep page sizes small.

Finally, the range_with_values method on the Recon trait was removed as it was unused

This change removes the offset/limit pagination pattern where it implies
a more flexible API than was needed or used. Having the flexible API
meant that casts from usize to smaller types for limits and offsets
could break pagination assumptions. Now with this change more specific
APIs are exposed where its safe to make assumptions about the size of
limit offset values in queries.

Additionally in the API where true pagination was the desired API
u32 is used instead of usize to keep page sizes small.

Finally, the range_with_values method on the Recon trait was removed as
it was unused
@nathanielc nathanielc requested review from a team and dav1do as code owners December 4, 2024 16:59
@nathanielc nathanielc requested review from 3benbox and removed request for a team December 4, 2024 16:59
@nathanielc nathanielc changed the title refactor: remove offset/limit where to generic refactor: remove offset/limit where too generic Dec 4, 2024
Copy link
Contributor

@dav1do dav1do left a comment

Choose a reason for hiding this comment

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

Nice, looks like an improvement. Thanks for doing this.

@nathanielc nathanielc enabled auto-merge December 4, 2024 20:22
@nathanielc nathanielc added this pull request to the merge queue Dec 4, 2024
Merged via the queue into main with commit c479255 Dec 4, 2024
5 checks passed
@nathanielc nathanielc deleted the fix/i64-limits branch December 4, 2024 21:22
@smrz2001 smrz2001 mentioned this pull request Dec 9, 2024
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.

2 participants