search

Textualize / textual

The lean application framework for Python. Build sophisticated user interfaces with a simple Python API. Run your apps in the terminal and a web browser.
https://textual.textualize.io/
MIT License
25.6k stars 788 forks source link

SelectionList values must be hashable for no “good” reason #3908

Open rodrigogiraoserrao opened 10 months ago

rodrigogiraoserrao commented 10 months ago

As it stands, if you try to create a SelectionList with values that are not hashable, you get an error. That's because we use a dictionary to keep track of the options that have been selected. Changing this to a set of indices should lift the seemingly arbitrary restriction that the values must be hashable while also making it straightforward & fast to do the selection bookkeeping and to return a sorted list of selections to the user.

TomJGooding commented 9 months ago

I might be completely off base here, but is this now resolved with the recent changes to OptionList?

rodrigogiraoserrao commented 9 months ago

You had me wondering for a minute. It's not solved because it's the selection list that contains this dictionary: self._selected: dict[SelectionType, None] = {} inside SelectionList.__init__.

davep commented 8 months ago

As of the time of writing #4256 adds code that further leans on that hashable nature; either this should be the moment to tackle this, or when this does get tackled that further change should be taken into account.

rodrigogiraoserrao commented 8 months ago

@davep On the one hand I'm not aware of how you use the hashable nature of selection list values in the linked PR; on the other hand this issue isn't to say we must absolutely 100% lift this restriction!

When I opened the issue it looked simple enough to lift the restriction by changing a dictionary to use indices into a list instead of using values directly.

Maybe your PR is reasonable enough that this becomes a documented restriction and this issue can be closed. I'd be fine with that. 🤷

davep commented 8 months ago

In this PR (#4256), given that values were considered to be valid as dictionary keys anyway, it leans into that when it needs to create a method of mapping values back to the index of an item; so it's pretty much similar to what you highlight above only it actually has an associated value too.

I sense you're right that it deserves cleaning up unless there's a penalty in doing so; but I thought it prudent to mention it here for anyone who picks this up.

rodrigogiraoserrao commented 8 months ago

Understood. (I think a good paper trail is always a good idea.)