-
Notifications
You must be signed in to change notification settings - Fork 63
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
Fetch runs table without Bravado deserialization #1573
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1573 +/- ##
==========================================
- Coverage 79.97% 73.81% -6.16%
==========================================
Files 291 292 +1
Lines 14511 14233 -278
==========================================
- Hits 11605 10506 -1099
- Misses 2906 3727 +821
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
||
|
||
def iter_over_pages( | ||
*, iter_once: Callable[..., List[Any]], step: int, max_server_offset: int = 10000 |
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.
What do you think not pass callable here and always call get_single_page
inside? And rename it to get leaderboard_entries?
To make it not so general
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.
Maybe move max_server_offset
to const?
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.
Sure, I agree with your suggestion but as we're going to reimplement the pagination (move from offset + limit to starting_after
) it's probably not worth it right now as we're going to get rid of max_server_offset
.
I'm not sure about getting rid of callable here as this potentially abstracts the whole pagination scheme and simplifies a testing setup. I'll try to remember this suggestion to address that with the next Pull Request.
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.
LGTM
Before submitting checklist