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

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

Open
rodrigogiraoserrao opened this issue Dec 20, 2023 · 6 comments
Open
Labels
enhancement New feature or request Task

Comments

@rodrigogiraoserrao
Copy link
Contributor

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.

@rodrigogiraoserrao rodrigogiraoserrao added enhancement New feature or request Task labels Dec 20, 2023
@TomJGooding
Copy link
Contributor

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

@rodrigogiraoserrao
Copy link
Contributor Author

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
Copy link
Contributor

davep commented Mar 5, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor

davep commented Mar 5, 2024

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
Copy link
Contributor Author

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

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

No branches or pull requests

3 participants