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 pagination support to list #26

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jacob-keller
Copy link

This series adds support for handling the pagination Link headers when
listing patches. Doing this allows handling replies which contain more than
30 patches.

To do this, I propose switching from using urllib to using the python
requests library. Alternatively we could process the Link headers manually
and continue using urllib without the extra dependency. This is possible but
could be somewhat tricky to get correct.

I only implemented Link handling in the _list implementation since that one
was the most clear place where it was important. I'm not sure what other
API endpoints can return Link headers.

  • convert REST API to use requests library
  • Handle the pagination Link headers for _list requests

Switch the REST API implementation over to the requests python library.
This will make it easier to handle things like pagination in future
changes. This adds a new dependency to the pwclient. However this is a well
known python library, and is even mentioned in the patchwork API
documentation itself.

Signed-off-by: Jacob Keller <[email protected]>
The patchwork REST API defaults to sending a maximum of 30 items for API
requests which return a list. This makes pwclient list fail to return more
than 30 items when using the REST API.

To handle this, the API includes 'Link' headers in the response which
indicate whether there is more data and what URL the data is available at.

Processing the Link header is done automatically by the requests library.
Handle this in the _list implementation by checking whether the response
had a 'next' header, and request that data as well. This allows the
pwclient to list everything instead of being limited to 30 patches.

Signed-off-by: Jacob Keller <[email protected]>
@stephenfin
Copy link
Member

This looks mostly good. My one request is to avoid using requests if at all possible. Currently we have a single dependency, which is a backport of a stdlib library and only necessary on Python 3.7 (which is now EOL).It would be good to maintain that if possible. Given we only need to be concerned with our own implementation of the Link header, I'm hoping this won't be too big an ask.

@stephenfin
Copy link
Member

Also, you can fix the linters CI job by running pre-commit run -a.

@jacob-keller
Copy link
Author

Makes sense. I'll have to dig into it a bit to see how to parse. I started that way but found it difficult to parse correctly (I ended up getting double queries somehow). It shouldn't be too tricky to figure out tho.

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