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 count attribute in response #143

Merged

Conversation

kevinwmerritt
Copy link
Contributor

I removed the requirement to have a count attribute in the DRF list response.

This closes #136

@kevinwmerritt kevinwmerritt changed the title do not require count attribute in response Do not require count attribute in response Oct 21, 2015
@benkonrath benkonrath self-assigned this Oct 21, 2015
@benkonrath
Copy link
Collaborator

Thanks @kevinwmerritt. I quickly scanned through this and I think I prefer a unit test rather than a full integration test which is more or less a copy of the pagination tests. I can write the test but I can I only do it in a couple of days.

@@ -104,6 +104,26 @@ export default DRFSerializer.extend({
});
```

## Cursor Pagination

To use `CursorPagination`, override `extractPageNumber` in the serializer to extract the `cursor`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you link this to the DRF docs on cursor pagination? ☝️

http://www.django-rest-framework.org/api-guide/pagination/#cursorpagination

@dustinfarris
Copy link
Owner

Thanks for this—and especially for adding to the docs!

I agree with Ben, the duplicated acceptance test module with count removed is probably overkill. I would just copy this unit test and remove count: https://github.com/dustinfarris/ember-django-adapter/blob/master/tests/unit/serializers/drf-test.js#L8

@kevinwmerritt kevinwmerritt force-pushed the allow-cursor-pagination branch from 3ebfb59 to 0e021a6 Compare October 21, 2015 14:26
@kevinwmerritt
Copy link
Contributor Author

@benkonrath I updated the PR with @dustinfarris's suggestion to use a unit test instead of the acceptance test.

@kevinwmerritt kevinwmerritt force-pushed the allow-cursor-pagination branch from 0e021a6 to a03d135 Compare October 21, 2015 14:32
@dustinfarris
Copy link
Owner

LGTM

dustinfarris added a commit that referenced this pull request Oct 22, 2015
Do not require count attribute in response
@dustinfarris dustinfarris merged commit d1d74ec into dustinfarris:master Oct 22, 2015
@dustinfarris
Copy link
Owner

@kevinwmerritt for some reason I thought I had your contact information already, but it appears I don't. Would you mind shooting me an email? [email protected]

@benkonrath
Copy link
Collaborator

I just checked it over too - looks good. Thanks guys!

@dustinfarris
Copy link
Owner

👍

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.

3 participants