chipx86 got a fish trophy!

Summary

Add initial support for paginators for Hosting Service requests.

Review Request #5885 — Created May 27, 2014 and submitted May 28, 2014, 4:03 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

880ed5a...

Reviewers

Groups

reviewboard

People

Description

We have functions in HostingService subclasses that need to deal with

paginated data. Right now, they're all handling it in their own ways,

but as we move to wrapping more of these in the web API, it's helpful to

standardize them.
This introduces a set of paginators that can be used and subclassed by

the more specific HostingServices to provide a standardized API for

pagination of data.
APIPaginator is designed to perform API requests and handle pagination

through links returned through the payload or headers in some form.

Subclasses are expected to define how API requests are made and parsed.
ProxyPaginator attaches to another paginator (typically an APIPaginator)

and transforms the results. This allows, for instance, a

HostingServiceClient to return an APIPaginator with raw results, and a

HostingService to wrap it and return structured data derived from those

raw results.

Testing Done

Unit tests passed.

Tested this manually with an accompanying change to GitHub's
get_remote_repositories. I was able to page back and forth and get the
results. I also tested start= and per_page= with that.

Issues

Description	From	Last Updated
What is the client parameter? It doesn't seem to be used.	david	May 27, 2014, 5:42 p.m.
typo (frmo)	david	May 27, 2014, 5:42 p.m.
Is this supposed to be public API?	david	May 28, 2014, 12:47 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/hostingsvcs/utils/tests.py
    reviewboard/hostingsvcs/utils/paginator.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/hostingsvcs/utils/tests.py
    reviewboard/hostingsvcs/utils/paginator.py
  Ignored Files:

reviewboard/hostingsvcs/utils/paginator.py (Diff revision 1)

The issue has been resolved. Show all issues

What is the client parameter? It doesn't seem to be used.

chipx86 May 27, 2014, 5:37 p.m.

Since subclasses need to make queries on a client (using json_get or api_get or whatever is appropriate), this standardizes accepting it as a parameter, so that subclasses don't have to override the constructor and accept it. The class is useless without it.

david May 27, 2014, 5:40 p.m.

Care to document that it's a HostingServiceClient and is used only in subclasses?

reviewboard/hostingsvcs/utils/paginator.py (Diff revision 1)
The issue has been resolved. Show all issues
```
typo (frmo)
```

Change Summary:


Improved documentation for APIPaginator, specifically to discuss the expected parameters and access to the HostingServiceClient.
When computing the URL and attaching query parameters, the parameters are now sorted by key. This helps a lot with unit tests and just keeping requests consistent.
Fixed a typo.

Commit:

4f83fcb6d0b13d58cc736b4d400bb410e41ecf03

b073f1653c3936e033ef7a7c20cc32d22f73eefd

Diff:

Revision 2 (+475)

Show changes

	reviewboard/hostingsvcs/utils/paginator.py
	reviewboard/hostingsvcs/utils/tests.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/hostingsvcs/utils/tests.py
    reviewboard/hostingsvcs/utils/paginator.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/hostingsvcs/utils/tests.py
    reviewboard/hostingsvcs/utils/paginator.py
  Ignored Files:

reviewboard/hostingsvcs/utils/paginator.py (Diff revision 2)

The issue has been resolved. Show all issues

Is this supposed to be public API?

chipx86 May 27, 2014, 7:23 p.m.

Yeah, it's available for subclassing if a paginator needs to be more complex in its processing. (I think we'll have some like that.)

I can mention it in the docstring.

Change Summary:

Improved a docstring.

Commit:

b073f1653c3936e033ef7a7c20cc32d22f73eefd

880ed5a08165ae35395c8e241335466923c3c953

Diff:

Revision 3 (+479)

Show changes

	reviewboard/hostingsvcs/utils/paginator.py
	reviewboard/hostingsvcs/utils/tests.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/hostingsvcs/utils/tests.py
    reviewboard/hostingsvcs/utils/paginator.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/hostingsvcs/utils/tests.py
    reviewboard/hostingsvcs/utils/paginator.py
  Ignored Files:

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (8ed4055)