Allow column to be ordered by ascending by default #566
Conversation
There was a problem hiding this comment.
Hi @karech, interesting use case, thanks for the pull!
Just to be sure, doesn't using order_by='-name' give the desired result?
Anyway, we need to have this properly documented (at least in the Column docstring and tested before it's acceptable.
django_tables2/columns/base.py
Outdated
| if self.column.default_descending_ordering: | ||
| order_by.next = order_by if self.is_ordered else order_by.opposite | ||
| else: | ||
| order_by.next = order_by.opposite if self.is_ordered else order_by |
There was a problem hiding this comment.
what about:
order_by.next = order_by.opposite if self.is_ordered else order_by
if self.column.default_descending_ordering:
order_by.next = order_by.opposite| @@ -111,7 +113,7 @@ class Column(object): | |||
| def __init__(self, verbose_name=None, accessor=None, default=None, | |||
| visible=True, orderable=None, attrs=None, order_by=None, | |||
| empty_values=None, localize=None, footer=None, | |||
| exclude_from_export=False): | |||
| exclude_from_export=False, default_descending_ordering=False): | |||
There was a problem hiding this comment.
Naming is certainly a good question.
default_ordering_desc?
default_ordering='-' for descending?
There was a problem hiding this comment.
I'm not sure about "default", because this only affects BoundColumn.order_by_alias.next.
Maybe first_ordering_descending=True ?
|
@jieter Let me try to explain, what I wanna achieve. I have tables, and custom columns (lets say NumberColumn, DecimalColumn(NunberColumn) and etc). And I wanna them always be sorted by descending on first user click. class NumberColumn(django_tables2.Column):
def __init__(*args, **kwargs):
super().__init__(*args, **kwargs)
self.default_descending_ordering = TrueAnd on first click on column they (columns with numbers) will be sorted by descending, but other columns (like text columns) - by ascending. |
There was a problem hiding this comment.
@karech implementation looks good now, but before I'm going to merge this, it needs to have a good test. Especially with these less commonly used features, it's easy for a future contributor to break it accidentally if no tests cover it.
Let me know if you need any help.
| @@ -96,6 +96,8 @@ class Column(object): | |||
| - If `True`, force localization | |||
| - If `False`, values are not localized | |||
| - If `None` (default), localization depends on the ``USE_L10N`` setting. | |||
| default_descending_ordering (bool): If 'True', | |||
| column.order_by_alias.next will return descending ordering if not ordered. | |||
There was a problem hiding this comment.
Can you add to this sentence that it defaults to False?
|
@jieter Hello! I'v added tests (tried to make same styling as other tests). And I'm still not sure about naming of parameter. Looks not very clear. |
| table_desc = TableDescOrd(data) | ||
|
|
||
| assert table.columns[1].order_by_alias.next == 'field' | ||
| assert table_desc.columns[1].order_by_alias.next == '-field' |
There was a problem hiding this comment.
Can use a TestCase-derived test class? Using its assertion methods (like self.assertEqual()) results in much more informative failed assertion messages.
|
Closing this after merging #597 |
Not sure about: