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

Support custom sort functions in 'DataTable' #2512

Closed
wants to merge 1 commit into from

Conversation

Alphix
Copy link

@Alphix Alphix commented May 8, 2023

This is somewhat related to issue #2261

Please review the following checklist.

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

@Alphix Alphix force-pushed the data-table-custom-sort branch from b43a384 to 06b5293 Compare May 8, 2023 12:47
@Alphix Alphix force-pushed the data-table-custom-sort branch from 06b5293 to da2dc9f Compare May 8, 2023 12:48
@willmcgugan
Copy link
Collaborator

@darrenburns Thoughts on this?

@darrenburns
Copy link
Member

I like the idea but I'm wondering if sorting by key should be a separate method. With the approach in this PR, "column" and "key" have an awkward dependency on each other. If you supply both, it's unclear what will happen without reading the docs.

@Alphix
Copy link
Author

Alphix commented May 31, 2023

I like the idea but I'm wondering if sorting by key should be a separate method. With the approach in this PR, "column" and "key" have an awkward dependency on each other. If you supply both, it's unclear what will happen without reading the docs.

I could change the PR to follow that approach. I guess the alternative method (sort_custom()?) would have to duplicate most of the body of the current sort() method, but that's only 9 lines of duplication...

@darrenburns
Copy link
Member

Another option could be that the sort method signature is updated from

    def sort(
        self,
        *columns: ColumnKey | str,
        reverse: bool = False,
    ) -> Self

To something like...

    def sort(
        self,
        by: Iterable[ColumnKey | str] | Callable,
        reverse: bool = False,
    ) -> Self

So you could either supply an iterable of columns or you can supply a function to sort.

joshbduncan added a commit to joshbduncan/textual that referenced this pull request 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.
darrenburns added a commit that referenced this pull request 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 PR looking for something else.

Sorting a DataTable with a custom function was added in Textual v0.41.0. - https://textual.textualize.io/widgets/data_table/#sorting

@Alphix
Copy link
Author

Alphix commented Jan 11, 2024

I stumbled across this open PR looking for something else.

Sorting a DataTable with a custom function was added in Textual v0.41.0. - https://textual.textualize.io/widgets/data_table/#sorting

Indeed, I'll close this...

@Alphix Alphix closed this Jan 11, 2024
@Alphix Alphix deleted the data-table-custom-sort branch January 11, 2024 08:25
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.

4 participants