-
Notifications
You must be signed in to change notification settings - Fork 780
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
Adds an interface for replacing prompt of an individual option in an OptionList
#2985
Changes from all commits
45b6973
37bc2e5
22926cb
a3590ac
d63ac2c
913dce7
3ff5b7b
def2628
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,14 @@ def prompt(self) -> RenderableType: | |
"""The prompt for the option.""" | ||
return self.__prompt | ||
|
||
def set_prompt(self, prompt: RenderableType) -> None: | ||
"""Set the prompt for the option. | ||
|
||
Args: | ||
prompt: The new prompt for the option. | ||
""" | ||
self.__prompt = prompt | ||
|
||
@property | ||
def id(self) -> str | None: | ||
"""The optional ID for the option.""" | ||
|
@@ -619,12 +627,7 @@ def remove_option(self, option_id: str) -> Self: | |
Raises: | ||
OptionDoesNotExist: If no option has the given ID. | ||
""" | ||
try: | ||
self._remove_option(self._option_ids[option_id]) | ||
except KeyError: | ||
raise OptionDoesNotExist( | ||
f"There is no option with an ID of '{option_id}'" | ||
) from None | ||
self._remove_option(self.get_option_index(option_id)) | ||
return self | ||
|
||
def remove_option_at_index(self, index: int) -> Self: | ||
|
@@ -647,6 +650,54 @@ def remove_option_at_index(self, index: int) -> Self: | |
) from None | ||
return self | ||
|
||
def _replace_option_prompt(self, index: int, prompt: RenderableType) -> None: | ||
"""Replace the prompt of an option in the list. | ||
|
||
Args: | ||
index: The index of the option to replace the prompt of. | ||
prompt: The new prompt for the option. | ||
|
||
Raises: | ||
OptionDoesNotExist: If there is no option with the given index. | ||
""" | ||
self.get_option_at_index(index).set_prompt(prompt) | ||
self._refresh_content_tracking(force=True) | ||
self.refresh() | ||
|
||
def replace_option_prompt(self, option_id: str, prompt: RenderableType) -> Self: | ||
"""Replace the prompt of the option with the given ID. | ||
|
||
Args: | ||
option_id: The ID of the option to replace the prompt of. | ||
prompt: The new prompt for the option. | ||
|
||
Returns: | ||
The `OptionList` instance. | ||
|
||
Raises: | ||
OptionDoesNotExist: If no option has the given ID. | ||
""" | ||
self._replace_option_prompt(self.get_option_index(option_id), prompt) | ||
return self | ||
|
||
def replace_option_prompt_at_index( | ||
self, index: int, prompt: RenderableType | ||
) -> Self: | ||
"""Replace the prompt of the option at the given index. | ||
|
||
Args: | ||
index: The index of the option to replace the prompt of. | ||
prompt: The new prompt for the option. | ||
|
||
Returns: | ||
The `OptionList` instance. | ||
|
||
Raises: | ||
OptionDoesNotExist: If there is no option with the given index. | ||
""" | ||
self._replace_option_prompt(index, prompt) | ||
return self | ||
|
||
def clear_options(self) -> Self: | ||
"""Clear the content of the option list. | ||
|
||
|
@@ -720,12 +771,7 @@ def enable_option(self, option_id: str) -> Self: | |
Raises: | ||
OptionDoesNotExist: If no option has the given ID. | ||
""" | ||
try: | ||
return self.enable_option_at_index(self._option_ids[option_id]) | ||
except KeyError: | ||
raise OptionDoesNotExist( | ||
f"There is no option with an ID of '{option_id}'" | ||
) from None | ||
return self.enable_option_at_index(self.get_option_index(option_id)) | ||
|
||
def disable_option(self, option_id: str) -> Self: | ||
"""Disable the option with the given ID. | ||
|
@@ -739,12 +785,7 @@ def disable_option(self, option_id: str) -> Self: | |
Raises: | ||
OptionDoesNotExist: If no option has the given ID. | ||
""" | ||
try: | ||
return self.disable_option_at_index(self._option_ids[option_id]) | ||
except KeyError: | ||
raise OptionDoesNotExist( | ||
f"There is no option with an ID of '{option_id}'" | ||
) from None | ||
return self.disable_option_at_index(self.get_option_index(option_id)) | ||
|
||
@property | ||
def option_count(self) -> int: | ||
|
@@ -761,7 +802,7 @@ def get_option_at_index(self, index: int) -> Option: | |
The option at that index. | ||
|
||
Raises: | ||
OptionDoesNotExist: If there is no option with the index. | ||
OptionDoesNotExist: If there is no option with the given index. | ||
""" | ||
try: | ||
return self._options[index] | ||
|
@@ -779,11 +820,22 @@ def get_option(self, option_id: str) -> Option: | |
Returns: | ||
The option with the ID. | ||
|
||
Raises: | ||
OptionDoesNotExist: If no option has the given ID. | ||
""" | ||
return self.get_option_at_index(self.get_option_index(option_id)) | ||
|
||
def get_option_index(self, option_id): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're introducing this as part of the public interface, and in service of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should mention, same for other similar developer-facing methods. If that's what #2986 was in reference to then I'm in favour of that and would support it. |
||
"""Get the index of the option with the given ID. | ||
|
||
Args: | ||
option_id: The ID of the option to get the index of. | ||
|
||
Raises: | ||
OptionDoesNotExist: If no option has the given ID. | ||
""" | ||
try: | ||
return self.get_option_at_index(self._option_ids[option_id]) | ||
return self._option_ids[option_id] | ||
except KeyError: | ||
raise OptionDoesNotExist( | ||
f"There is no option with an ID of '{option_id}'" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice testing. One thing I would have done here and I think we should do this is take into account that a prompt can span multiple lines. So I'd write some extra replacement tests where a prompt goes from more to fewer lines, and from fewer to more lines. I'd also add some snapshot tests too, perhaps:
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
"""Test replacing options prompt from an option list.""" | ||
nekeal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import pytest | ||
|
||
from textual.app import App, ComposeResult | ||
from textual.widgets import OptionList | ||
from textual.widgets.option_list import Option, OptionDoesNotExist | ||
|
||
|
||
class OptionListApp(App[None]): | ||
"""Test option list application.""" | ||
|
||
def compose(self) -> ComposeResult: | ||
yield OptionList( | ||
Option("0", id="0"), | ||
Option("line1\nline2"), | ||
) | ||
|
||
|
||
async def test_replace_option_prompt_with_invalid_id() -> None: | ||
"""Attempting to replace the prompt of an option ID that doesn't exist should raise an exception.""" | ||
async with OptionListApp().run_test() as pilot: | ||
with pytest.raises(OptionDoesNotExist): | ||
pilot.app.query_one(OptionList).replace_option_prompt("does-not-exist", "new-prompt") | ||
|
||
|
||
async def test_replace_option_prompt_with_invalid_index() -> None: | ||
"""Attempting to replace the prompt of an option index that doesn't exist should raise an exception.""" | ||
async with OptionListApp().run_test() as pilot: | ||
with pytest.raises(OptionDoesNotExist): | ||
pilot.app.query_one(OptionList).replace_option_prompt_at_index(23, "new-prompt") | ||
|
||
|
||
async def test_replace_option_prompt_with_valid_id() -> None: | ||
"""It should be possible to replace the prompt of an option ID that does exist.""" | ||
async with OptionListApp().run_test() as pilot: | ||
option_list = pilot.app.query_one(OptionList) | ||
option_list.replace_option_prompt("0", "new-prompt") | ||
assert option_list.get_option("0").prompt == "new-prompt" | ||
|
||
|
||
async def test_replace_option_prompt_with_valid_index() -> None: | ||
"""It should be possible to replace the prompt of an option index that does exist.""" | ||
async with OptionListApp().run_test() as pilot: | ||
option_list = pilot.app.query_one(OptionList).replace_option_prompt_at_index(1, "new-prompt") | ||
assert option_list.get_option_at_index(1).prompt == "new-prompt" | ||
|
||
|
||
async def test_replace_single_line_option_prompt_with_multiple() -> None: | ||
"""It should be possible to replace single line prompt with multiple lines """ | ||
new_prompt = "new-prompt\nsecond line" | ||
async with OptionListApp().run_test() as pilot: | ||
option_list = pilot.app.query_one(OptionList) | ||
option_list.replace_option_prompt("0", new_prompt) | ||
assert option_list.get_option("0").prompt == new_prompt | ||
|
||
|
||
async def test_replace_multiple_line_option_prompt_with_single() -> None: | ||
"""It should be possible to replace multiple line prompt with a single line""" | ||
new_prompt = "new-prompt" | ||
async with OptionListApp().run_test() as pilot: | ||
option_list = pilot.app.query_one(OptionList) | ||
option_list.replace_option_prompt("0", new_prompt) | ||
assert option_list.get_option("0").prompt == new_prompt | ||
|
||
|
||
async def test_replace_multiple_line_option_prompt_with_multiple() -> None: | ||
"""It should be possible to replace multiple line prompt with multiple lines""" | ||
new_prompt = "new-prompt\nsecond line" | ||
async with OptionListApp().run_test() as pilot: | ||
option_list = pilot.app.query_one(OptionList) | ||
option_list.replace_option_prompt_at_index(1, new_prompt) | ||
assert option_list.get_option_at_index(1).prompt == new_prompt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also document what gets raised if the option doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary if it's already documented in the
get_option_index()
method?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say "yes" because users will see the documentation for
replace_option_prompt
and they won't look at how it is implemented to figure out what may get raised.This will also make sure this method is more aligned with all others, because all other methods say they will raise the error. It's just that we raise it a bit later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, you are right. I added
Raises
section.