Skip to content

fix: add set_options to SelectionList#6224

Open
NSPC911 wants to merge 1 commit into
Textualize:mainfrom
NSPC911-forks:selection-set-options-clear-selected
Open

fix: add set_options to SelectionList#6224
NSPC911 wants to merge 1 commit into
Textualize:mainfrom
NSPC911-forks:selection-set-options-clear-selected

Conversation

@NSPC911

@NSPC911 NSPC911 commented Nov 11, 2025

Copy link
Copy Markdown
Contributor

threw me off when this happened to rovr, because the self._selected never got cleared when set_options was used instead of clear_options + add_options

Please review the following checklist.

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

threw me off when this happened to rovr, because the self._selected
never got cleared when `set_options` was used instead of `clear_options`
+ `add_options`
NSPC911 added a commit to NSPC911/rovr that referenced this pull request Nov 11, 2025
waiting for a new release that has
Textualize/textual#6224 merged

@TomJGooding TomJGooding left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is correct? You can build a SelectionList with tuples or Selection objects, but here it looks like you're trying to use SelectionType?

@NSPC911

NSPC911 commented Nov 11, 2025

Copy link
Copy Markdown
Contributor Author

Are you sure this is correct? You can build a SelectionList with tuples or Selection objects, but here it looks like you're trying to use SelectionType?

not really sure which one to use, given that OptionList's set_option method only takes in an Iterable OptionListContent

@TomJGooding

Copy link
Copy Markdown
Contributor

I'm confused, Did you check your code actually worked, or just throw the PR over the wall?

@NSPC911

NSPC911 commented Nov 12, 2025

Copy link
Copy Markdown
Contributor Author

The general code worked, but I'm not sure about which class to use as the type hint for the function

@TomJGooding

Copy link
Copy Markdown
Contributor

Sorry, I think I understand the problem now.

I thought the signature would just be similar to the SelectionList.add_options method:

def add_options(
self,
items: Iterable[
OptionListContent
| Selection[SelectionType]
| tuple[ContentText, SelectionType]
| tuple[ContentText, SelectionType, bool]
],
) -> Self:

But of course this doesn't satisfy mypy and friends for super().set_options(options).

I realise now that I overlooked that this actually ends up calling SelectionList.add_options. Repeating the same runtime checks in both set_option and add_options would definitely be "sort of sub-optimal"!

@NSPC911

NSPC911 commented Apr 19, 2026

Copy link
Copy Markdown
Contributor Author

@willmcgugan really sorry to ping you but can you take a look at this PR? Textual currently assumes that when you call set_options, the values or selected options' values are not reset, which means it is using an outdated dictionary, and hence will cause issues when parsing the selected values.

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.

2 participants