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

Sort method inDataTable doesn't support rich.Text types #2261

Closed
msempere opened this issue Apr 11, 2023 · 13 comments
Closed

Sort method inDataTable doesn't support rich.Text types #2261

msempere opened this issue Apr 11, 2023 · 13 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@msempere
Copy link

Coming from #2243.

The sort method inDataTable doesn't support rich.Text types stored in the cells. When that is done a TypeError: '<' not supported between instances of 'Text' and 'Text' error is thrown because rich.Text have no rich comparison operators defined.

cc @rodrigogiraoserrao

@github-actions
Copy link

We found the following entries in the FAQ which you may find helpful:

Feel free to close this issue if you found an answer in the FAQ. Otherwise, please give us a little time to review.

This is an automated reply, generated by FAQtory

@rodrigogiraoserrao rodrigogiraoserrao added enhancement New feature or request question Further information is requested labels Apr 11, 2023
@rodrigogiraoserrao
Copy link
Contributor

My original thought (from the PR linked above):

“Maybe we could add a key parameter to the sort methods, akin to how sorted(..., key=...) works.”

@darrenburns might be better suited to determine if this makes sense.

@willmcgugan
Copy link
Collaborator

A key method makes sense. But I have no issue adding comparison methods to Text which just treat it like a string.

@msempere
Copy link
Author

A key method makes sense. But I have no issue adding comparison methods to Text which just treat it like a string.

@willmcgugan adding them to Text is probably the best idea. I'll work on that.
Could you assign that issue to me? thanks!

Alphix added a commit to Alphix/textual that referenced this issue May 8, 2023
Alphix added a commit to Alphix/textual that referenced this issue May 8, 2023
Alphix added a commit to Alphix/textual that referenced this issue May 8, 2023
joshbduncan added a commit to joshbduncan/textual that referenced this issue Aug 11, 2023
The `DataTable` widget now takes the `by` argument instead of `columns`, allowing the table to also be sorted using a custom function (or other callable). This is a breaking change since it requires all calls to the `sort` method to include an iterable of key(s) (or a singular function/callable). Covers Textualize#2261 using [suggested function signature](Textualize#2512 (comment)) from @darrenburns on PR Textualize#2512.
@candidtim
Copy link

candidtim commented Sep 12, 2023

Only having the ordering functions on the rich.text.Text won't be enough for all use cases (although I entirely support this change too). For example: I want to sort a column with humanized numbers (e.g., file sizes like "42 Bytes" and "2 MiB"). It needs to be sorted based on the raw values themselves, independently of their representation.

Adding a sort_fn to the add_column method would not be helpful in this particular case either, because it implies that it is possible to convert the textual representation back into whatever can be ordered, which in practice is not always the case. But otherwise I think it is nice to have it for more flexibility anyway.

One possible solution to the above problem is to allow conveying the values and their representation separately. Then, either use the raw values for ordering, or also allow a custom function to extract the sort keys.

Meanwhile, for a workaround I extend the rich.text.Text to implement the ordering. For example:

@functools.total_ordering
class Sortable(Text):
    """Like rich.text.Text, but with ordering based on a raw value"""

    def __init__(self, value, *args, **kwargs):
        self.value = value
        super().__init__(*args, **kwargs)

    def __lt__(self, other: "Sortable") -> bool:
        return self.value < other.value

    def __eq__(self, other: "Sortable") -> bool:
        return self.value == other.value

and later use it like so:

    def watch_xyz(self, ...):
        ...
        self.table.add_row(
            ...
            Sortable(file_size, naturalsize(file_size), ...),
            ...
        )

It allows customizing the ordering logic in any way necessary by changing the __lt__ implementation. For example I did things like sticky top/bottom rows, etc.

@TomJGooding
Copy link
Contributor

Adding a sort_fn to the add_column method would not be helpful in this particular case either, because it implies that it is possible to convert the textual representation back into whatever can be ordered, which in practice is not always the case.

I'm assuming you've looked at #3090, could you give an example where this would not work?

@candidtim
Copy link

I'm assuming you've looked at #3090, could you give an example where this would not work?

As far as I understand the example from above still holds, but please let me know if I am missing something. If I understand the change well, the new key argument accepts a function which will be called with a row content (key and cell values). This means that if I want to sort based on some value, this value necessarily needs to be present in the row cells in one way or another. What if the value I want to sort on is not directly present in the row? For example, I would show humanized numbers ("1 MiB", "1 GiB") in the column, but would want to sort it based on actual numbers (1 MiB < 1 GiB). Since the actual number is not present in the row, how would the key function obtain it?

@sharkusk
Copy link

In the sort routine you can always un-humanize the numbers. You might get some round-off errors if you don't have enough precision in your human readable numbers, but in general works pretty well. (I do this in my app today with a DataTable and a custom sort() function.)

@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Sep 13, 2023

How would the key function obtain it?

The key function can do whatever you want with the values it receives, namely “un-humanize” them.
See, for example, the parameter key in the built-in sorted: https://docs.python.org/3/library/functions.html#sorted

@guystreeter
Copy link

I would very much like to supply my own callback to the sort function. Simply handing me the row key would be enough, but having all the row data would be better.
An alternative would be enabling a hidden column. I could put a sortable value there that doesn't get displayed.

darrenburns added a commit that referenced this issue Oct 31, 2023
* DataTable sort by function (or other callable)

The `DataTable` widget now takes the `by` argument instead of `columns`, allowing the table to also be sorted using a custom function (or other callable). This is a breaking change since it requires all calls to the `sort` method to include an iterable of key(s) (or a singular function/callable). Covers #2261 using [suggested function signature](#2512 (comment)) from @darrenburns on PR #2512.

* argument change and functionaloty update

Changed back to orinal `columns` argument and added a new `key` argument
which takes a function (or other callable). This allows the PR to NOT BE
a breaking change.

* better example for docs

- Updated the example file for the docs to better show the functionality
of the change (especially when using `columns` and `key` together).
- Added one new tests to cover a similar situation to the example
  changes

* removed unecessary code from example

- the sort by clicked column function was bloat in my opinion

* requested changes

* simplify method and terminology

* combine key_wrapper and default sort

* Removing some tests from DataTable.sort as duplicates. Ensure there is test coverage of the case where a key, but no columns, is passed to DataTable.sort.

* Remove unused import

* Fix merge issues in CHANGELOG, update DataTable sort-by-key changelog PR link

---------

Co-authored-by: Darren Burns <[email protected]>
Co-authored-by: Darren Burns <[email protected]>
@TomJGooding
Copy link
Contributor

I stumbled across this open issue looking for something else. You can now sort your DataTable with a custom function (or other callable) in Textual v0.41.0 - see https://textual.textualize.io/widgets/data_table/#sorting

@rodrigogiraoserrao
Copy link
Contributor

Thanks @TomJGooding, looks like we can close this issue now.

Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

7 participants