Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Improvement/revamp cursor iteration #44
base: master
Are you sure you want to change the base?
Improvement/revamp cursor iteration #44
Changes from all commits
20a43a1
61dc775
5915e13
ae6993d
18dd233
60b82b7
8c671cb
2deb232
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can't we just keep
num_elements
, and entirely removecount
from the method?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 didn't address this comment.
I remember trying hard to handle all corners cases and also avoiding all these intermediate variables and always bumping into a problem or having a pretty unreadable code.
I think the code is both functional and maintanable as is so I would keep it that way.
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'm not sure to understand why you
mock_ordered_cursor(..., start=skip, ...)
and thencursor.skip(skip)
, can you explain?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.
The goal of this test and the ones following is to validate that the cursor is iterating over pages properly taking
skip
andlimit
parmeters into account. These parameters are setting up what is going to be asked of the API, not doing some post-request manipulation of the results.The
start
,count
orfactor
arguments given tomock_ordered_cursor
are used to simulate the expected response from the API. Therefore, we need to align what we want (ie. setting up the cursor) with what is expected to be returned by theclient.get
method that we are patching, feedinggen_collection
results to it.I think the tests are ok as is (used them to validate the algo changes) though I admit that they could be a bit better.
One potentially better approach would be to actually monkey patch
request.get
and similar to change the raw results based on the query params of the url requested. I felt this was a bit overkill for what I wanted to achieve here.