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

Add support for iterating over lists with a generator. #186

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

Conversation

tim-schilling
Copy link

This requires a thorough review. I spotted two bugs in which the get_all url didn't match the other url. The bugs were that the one url didn't use the plural version.

@stephenross
Copy link
Collaborator

Looking at this set of changes, it looks like a great addition. I see you did make some stylistic changes with how you handled using the _build_path() calls by making the call as a variable assignment when it has additional arguments outside of the attributes on the main class, which I believe would be very useful on all endpoint calls, and not just calls to the all() endpoint methods. I do see what appears to be a slight difference between the README.md and README.rst information in at least the Pagination section as well. Both of these changes could probably be additional expansions made later. Have any of these endpoint changes and updates been tested on an account that has at least 2 pages of data according to the default counts and offsets for the endpoints changed?

@tim-schilling
Copy link
Author

tim-schilling commented Dec 20, 2018

Ah, sorry about the stylistic changes. I did my best to stay consistent, but I had to make some changes to make it easier to implement the changes.

I did test all three variations with the lists API endpoint with my data. I'll be using this for one of my projects. I tested the pagination logic, but had to change the page_size as I don't have 1000 records for any endpoint.

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