-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[ENH] Fix for record pagination #1450
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
I'd like us to add a property test that applies valid limits/offsets and does a model check. I also think we ought to test the tail cases - what happens if limit is negative, offset is beyond size etc. |
generate random limit and offset (eg like
what happens if you limit negative |
this is ready to merge - pending review |
res = segment.get_metadata(limit=3) | ||
assert len(res) == 3 | ||
|
||
# if limit is negative, return all results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to just error? I think being explicit would be nice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ive made this change
clients/js/package.json
Outdated
@@ -43,7 +43,7 @@ | |||
"scripts": { | |||
"test": "run-s db:clean db:cleanauth db:run test:runfull db:clean test:runfull-authonly db:cleanauth", | |||
"testnoauth": "run-s db:clean db:run test:runfull db:clean", | |||
"testauth": "run-s db:cleanauth db:run-auth test:runfull-authonly db:cleanauth", | |||
"testauth": "run-s db:cleanauth test:runfull-authonly db:cleanauth", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me - why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was made upstream
clients/js/src/AdminClient.ts
Outdated
@@ -0,0 +1,272 @@ | |||
import { Configuration, ApiApi as DefaultApi } from "./generated"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was made upstream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused why the JS client changes are so large in scope, but this overall looks good to me minus some questions.
This diff got really messed up somehow - will fix and then have you re-review. |
@HammadB ok this is fixed up now, sorry about the weird diff, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great thanks
This fixes the pagination on records. Before we would select all data, and then subsample - this obviously is not very performant and this fixes it. credit to @HammadB