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

Refactor default_order. #16

Merged
merged 5 commits into from
Feb 25, 2014
Merged

Refactor default_order. #16

merged 5 commits into from
Feb 25, 2014

Conversation

hobbestigrou
Copy link
Contributor

Merge order_by_list and default_order.

@benoitbryon
Copy link
Contributor

Missing tests around order reverse (perhaps another story): as an example default_order = '-last_name'.

@benoitbryon
Copy link
Contributor

Missing tests when order_by is passed in request (request.GET['order_by'] is not empty).

@benoitbryon
Copy link
Contributor

Former order_by_list was a list, whereas default_order is a string... Was the "list" feature important? Perhaps not.

@benoitbryon
Copy link
Contributor

Missing notes in CHANGELOG.
Missing deprecation warnings too? (is it covered by deprecation policy?)

@hobbestigrou
Copy link
Contributor Author

I'll make a ticket for default_order can make a list or a string and remove order_reverse.

@Natim
Copy link
Contributor

Natim commented Feb 25, 2014

Then how do you manage the order arrow?

@benoitbryon
Copy link
Contributor

Have you checked the demo project? Does it explain the usage of default_order?

self.assertEqual(queryset.query.order_by, ['last_name'])

def test_default_order_form_invalid(self):
"""Queryset is ordered by default_order when no order_by in request."""
Copy link
Contributor

Choose a reason for hiding this comment

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

and form is invalid

boblefrag added a commit that referenced this pull request Feb 25, 2014
@boblefrag boblefrag merged commit 576706f into master Feb 25, 2014
@boblefrag boblefrag deleted the 16-default_order branch February 25, 2014 10:45
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.

None yet

4 participants