- Notifications
You must be signed in to change notification settings - Fork673
python-gitlab Issue #63 - implement pagination for list()#64
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -189,6 +189,8 @@ def set_url(self, url): | ||
| self._url = '%s/api/v3' % url | ||
| def _construct_url(self, id_, obj, parameters): | ||
| if 'next_url' in parameters: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Could you handle prev, first and last as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @gpocentek I'm not sure I understand the purpose of that, given how this function is called. Even if I don't use the generator idea, from@dserodio, it's your project but I'd rather not add in additional unused code paths... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. My comment was just plain stupid. Sorry. | ||
| return parameters['next_url'] | ||
| args = _sanitize_dict(parameters) | ||
| url = obj._url % args | ||
| if id_ is not None: | ||
| @@ -342,8 +344,13 @@ def list(self, obj_class, **kwargs): | ||
| if key in cls_kwargs: | ||
| del cls_kwargs[key] | ||
| results = [cls(self, item, **cls_kwargs) for item in r.json() | ||
| if item is not None] | ||
| if 'next' in r.links and 'url' in r.links['next']: | ||
| args = kwargs.copy() | ||
| args['next_url'] = r.links['next']['url'] | ||
| results.extend(self.list(obj_class, **args)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. This doesn't seem to give a choice to the user, this will always return the full list and do multiple API calls. If a user has hundreds of items this is a problem. It should be optional. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. IMHO, the perfect implementation would use Python generators instead of a list, and only fetch the next page when needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Sounds like a good plan. I'm a bit short on time to work on this these days but I'd be happy to merge this solution. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I'm not sure I see this as "a problem" - the whole reason why I opened this PR is because I have about 200 users, and will soon have about 300 repositories. I've seen some latency when fetching all that, but not much. I like@dserodio's idea of using a generator for this. I'm going to have a try at implementing it, though I haven't done much with generators before, and perhaps worse, I've never really worked with | ||
| return results | ||
| else: | ||
| _raise_error_from_response(r, GitlabListError) | ||