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

Do not require the count attribute to be present #136

Closed
wants to merge 1 commit into from

Conversation

kevinwmerritt
Copy link
Contributor

When using cursor pagination the count attribute is not included by default.
http://www.django-rest-framework.org/api-guide/pagination/#cursorpagination

This one line change is required for cursor pagination to work. The count attribute was not required before 1.0.

@kevinwmerritt
Copy link
Contributor Author

I misspoke. Before 1.0 I did have to override extractPageNumber in the serializer.

  extractPageNumber: function(url) {
    var match = /.*?[\?&]cursor=([A-Za-z0-9]+).*?/.exec(url);
    if (match) {
      return match[1];
    }
    return null;
  },

@benkonrath
Copy link
Collaborator

@kevinwmerritt Thanks for your PR and comments. Do you think there's a way to support both CursorPagination and PageNumberPagination without the implementations interfering too much?

@dustinfarris
Copy link
Owner

I think he's saying that both will work if the check for count is removed. Is that right, @kevinwmerritt?

@kevinwmerritt
Copy link
Contributor Author

@dustinfarris correct. Removing the check for count allows PageNumberPagination and CursorPagination to work out of the box.

@benkonrath
Copy link
Collaborator

Ok, got it. Thanks.

@kevinwmerritt Can you add a test to ensure the count check doesn't creep in again? One idea for this test is to test that normalizeResponse sets up the meta hash when count is present and not present. Let me know if you run into problems with writing the test and I'll try to help.

@benkonrath
Copy link
Collaborator

@kevinwmerritt It would also be nice to get your extractPageNumber override into the docs for CursorPagination if you're ok with it.

@dustinfarris
Copy link
Owner

@kevinwmerritt are you able to make the changes @benkonrath suggested?

@kevinwmerritt
Copy link
Contributor Author

@dustinfarris Yes. I think I can get to it this week. I can close this PR for now.

@dustinfarris
Copy link
Owner

Did you mean to close this?

@kevinwmerritt kevinwmerritt reopened this Oct 1, 2015
@dustinfarris
Copy link
Owner

Hi @kevinwmerritt. Any update here?

@kevinwmerritt
Copy link
Contributor Author

Closing in favor of #143

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants