Adds an interface for replacing prompt of an individual option in an `OptionList` #2985

nekeal · 2023-07-22T12:26:53Z

Please review the following checklist.

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

This PR extends OptionList class with an interface for replacing prompts in individual options.

Closes #2603

rodrigogiraoserrao

Looks good to me!
Let's see what CI thinks :P

BenMarkFrost · 2023-07-22T12:32:18Z

src/textual/widgets/_option_list.py

+        Raises:
+            IndexError: If there is no option of the given index.
+        """
+        option = self._options[index]


There is a get_option_at_index() function already, could we use that here (And other functions that already exist for retrieving options) for consistency?

Suggested change

option = self._options[index]

option = self.get_option_at_index(index)

That's a good idea, for sure.

Good catch.

This PR is really good, implements the feature required in the issue very well.

I feel like we can do better to avoid rewriting code that already exists here. What do you think @nekeal ?

@BenMarkFrost I pushed the changes. Should I replace it also in the _remove_option method?

Which part of the code do you mean? The one that retrieves the option's index?

@BenMarkFrost I pushed the changes. Should I replace it also in the _remove_option method?

Yes well spotted, I think that would be a good idea. I can also see it in _set_option_disabled()

Which part of the code do you mean? The one that retrieves the option's index?

This PR re-implements errors for retrieving options by index and ID, when they are already present in those dedicated functions:

get_option() (which gets the option by option_id), and get_option_by_index().

The implementation in my opinion could be as simple as

def replace_option_prompt_at_index(index: int .......): self.get_option_at_index(index).set_prompt(new_prompt) def replace_option_prompt(id: str .......): self.get_option(id).set_prompt(new_prompt)

This would leave error handling for non-existant options to their respective functions, and would mean we can remove the _replace_option_prompt() function (as long as we move the refresh functions to the end of both of these too).

What do you think?

Sounds good to me 🚀

Should I also change implementations of other methods so they use get_option() and get_option_by_index().

edit: Actually I still need to do refresh() and _refresh_content_tracking() so it can be invoked inside of the _replace_option_prompt(). Is it ok?

Only if you have time / feel like it would be in scope, I think we can happily create another issue for that if it's too much work!

But yes ideally it should all be consistent

I just pushed a commit which shows how the rest of the methods can be implemented. Could you have a look?

rodrigogiraoserrao

Consider addressing https://github.com/Textualize/textual/pull/2985/files#r1271291408.

tests/option_list/test_option_prompt_replacement.py

rodrigogiraoserrao

Minor docstring changes, please!

src/textual/widgets/_option_list.py

rodrigogiraoserrao · 2023-07-22T14:07:12Z

src/textual/widgets/_option_list.py

+            prompt: The new prompt for the option.
+
+        Returns:
+            The `OptionList` instance.


Can we also document what gets raised if the option doesn't exist?

Is it necessary if it's already documented in the get_option_index() method?

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.

Ok, you are right. I added Raises section.

rodrigogiraoserrao · 2023-07-22T14:08:41Z

@willmcgugan I'd appreciate your input on this.
Not sure how we feel about the added level of indirection, which can be seen if we compare the replace_ methods directly with the remove_ methods, for example.

Related to the discussion in #2986

willmcgugan

LGTM! I'd like @davep to have a quick look, as he wrote the original code...

davep · 2023-07-22T14:29:53Z

I'd like @davep to have a quick look, as he wrote the original code...

Will do. Likely won't be until tomorrow at the earliest, or Monday morning when I get to my desk at the latest. Very keen to look it over.

nekeal · 2023-07-22T14:34:16Z

Will do. Likely won't be until tomorrow at the earliest, or Monday morning when I get to my desk at the latest. Very keen to look it over.

There is not much to look at. Don't let me leave sprints without a single merged PR 😉

willmcgugan · 2023-07-22T15:20:03Z

LGTM. If tests pass, merge away!

rodrigogiraoserrao · 2023-07-22T20:33:49Z

src/textual/widgets/_option_list.py

+            prompt: The new prompt for the option.
+
+        Returns:
+            The `OptionList` instance.


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.

rodrigogiraoserrao · 2023-07-22T20:34:03Z

src/textual/widgets/_option_list.py

+        """
+        try:
+            return self._option_ids[option_id]
+        except:


Why the bare except clause?

No idea why I missed it. It's limited to KeyError now.

rodrigogiraoserrao · 2023-07-22T20:35:46Z

src/textual/widgets/_option_list.py

+            prompt: The new prompt for the option.
+
+        Raises:
+            IndexError: If there is no option of the given index.


I believe this is incorrect, right? What self.get_option_at_index might raise is OptionDoesNotExist.

Nice catch! Thank you for pointing that out. It's fixed now.

davep

This looks great! Hits the right interface, good testing, attention to the docs. Works for me!

I've suggested some small improvements, mainly revolving around making the testing as comprehensive as possible.

davep · 2023-07-23T07:17:10Z

src/textual/widgets/_option_list.py

@@ -789,6 +842,22 @@ def get_option(self, option_id: str) -> Option:
                f"There is no option with an ID of '{option_id}'"
            ) from None

+    def get_option_index(self, option_id):


If we're introducing this as part of the public interface, and in service of replace_option_prompt, I think I would reduce the duplication of code by tweaking remove_option to be based on it too. I think it makes sense to deduplicate the code in any developer-facing method that's unlikely to be called frequently.

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.

davep · 2023-07-23T07:28:16Z

tests/option_list/test_option_prompt_replacement.py

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:

Going from a single-line prompt to a different single-line prompt.

Going from many lines to fewer.

Going from one or more lines to more lines.

rodrigogiraoserrao · 2023-07-23T08:16:16Z

Hey @nekeal, I see you replied to my "request changes" but you haven't committed anything. I'm wondering if that's intentional 🤔
Either way, when you do commit your changes, please let us know if you intend to address @davep's suggestions or not!

Excellent work, so far!

nekeal · 2023-07-23T10:43:20Z

Hey @nekeal, I see you replied to my "request changes" but you haven't committed anything. I'm wondering if that's intentional thinking Either way, when you do commit your changes, please let us know if you intend to address @davep's suggestions or not!

Excellent work, so far!

No, it wasn't intentional 😅 Just slow WiFi in a train and I didn't notice that push failed.

Yeah, I will try to implement suggestions from @davep later today but I'm not sure if I'll get it working before the sprint ends. Are you ok with that?

rodrigogiraoserrao · 2023-07-25T10:45:15Z

Hey @nekeal, sorry for the time it took me to reply.
Supposedly, I had replied already but apparently I didn't – instead, I edited your last comment and added my reply there...
Which makes no sense 🤣

This is my original reply:

Yeah, I will try to implement suggestions from @davep later today but I'm not sure if I'll get it working before the sprint ends. Are you ok with that?

Absolutely. The sprint is an event to help people get started with in-person guidance. You are more than welcome to finish the PR “outside” the sprint, as you are absolutely more than welcome to keep contributing.

I see you committed something else in the meantime.
Are you happy for me to go over the PR again or do you need more time to finish addressing the feedback?

nekeal · 2023-07-25T10:47:49Z

Hey @nekeal, sorry for the time it took me to reply.
Supposedly, I had replied already but apparently I didn't – instead, I edited your last comment and added my reply there...
Which makes no sense 🤣

This is my original reply:

Yeah, I will try to implement suggestions from @davep later today but I'm not sure if I'll get it working before the sprint ends. Are you ok with that?

Absolutely. The sprint is an event to help people get started with in-person guidance. You are more than welcome to finish the PR “outside” the sprint, as you are absolutely more than welcome to keep contributing.

I see you committed something else in the meantime.
Are you happy for me to go over the PR again or do you need more time to finish addressing the feedback?

No problem at all 😅
I still need some time to add snapshot tests. I will l let you know when I'm finished.

rodrigogiraoserrao · 2023-07-25T11:16:26Z

Ok. When you are ready, feel free to re-request a review by clicking the arrows 🔄 next to my name at the top right.

nekeal · 2023-07-29T08:31:42Z

I've added some snapshot tests. I'm not sure if I should've created a new application for testing this particular case but let me know if I should reuse the existing one.

rodrigogiraoserrao

Looks good to me!

Sorry I took so long to review but I was away.

rodrigogiraoserrao · 2023-08-07T13:19:06Z

Hey @nekeal we're ready to merge this.
The only thing left to do is make sure the changelog is updated correctly.
Would you mind merging main into your branch and then making sure your changelog entry ends up at the top?

Thank you so much :D

This simplifies error handling in a public interface

nekeal · 2023-08-07T21:21:48Z

Hey @nekeal we're ready to merge this. The only thing left to do is make sure the changelog is updated correctly. Would you mind merging main into your branch and then making sure your changelog entry ends up at the top?

Thank you so much :D

No, problem. The branch is rebased with main now and hopefully ready to be merged 🚀

To allow for maintaining the location of the highlight as we rebuild the command list I'm probably going to need some method of tracking an ID for an option, so I can find its new index back. There's no method in OptionList right now for doing that; it's trivial, but it's not there. As it happens the changes in Textualize#2985 actually has that, so here I note that I'll look to making that happen once that gets added in.

willmcgugan · 2023-08-08T15:21:05Z

Thanks @nekeal !

rodrigogiraoserrao · 2023-08-08T15:31:32Z

@davep this has now been merged; I believe you wanted to use this elsewhere?

davep · 2023-08-08T16:00:34Z

Aye, will merge main into the command palette branch. Thanks all.

rodrigogiraoserrao approved these changes Jul 22, 2023

View reviewed changes

BenMarkFrost reviewed Jul 22, 2023

View reviewed changes

rodrigogiraoserrao suggested changes Jul 22, 2023

View reviewed changes

BenMarkFrost reviewed Jul 22, 2023

View reviewed changes

tests/option_list/test_option_prompt_replacement.py Show resolved Hide resolved

nekeal force-pushed the feature/extend-optionlist branch from 1c9f079 to 555b4d2 Compare July 22, 2023 13:36

BenMarkFrost mentioned this pull request Jul 22, 2023

Refactor _option_list.py to consistently use accessors #2986

Closed

rodrigogiraoserrao suggested changes Jul 22, 2023

View reviewed changes

willmcgugan approved these changes Jul 22, 2023

View reviewed changes

nekeal force-pushed the feature/extend-optionlist branch from 68490e9 to 10c8b5f Compare July 22, 2023 14:16

rodrigogiraoserrao self-requested a review July 22, 2023 20:30

rodrigogiraoserrao suggested changes Jul 22, 2023

View reviewed changes

davep approved these changes Jul 23, 2023

View reviewed changes

nekeal requested review from rodrigogiraoserrao July 29, 2023 08:29

rodrigogiraoserrao approved these changes Aug 7, 2023

View reviewed changes

Add ability to replace prompt of an individual option in OptionList

45b6973

nekeal force-pushed the feature/extend-optionlist branch from 6633a85 to 978e0a3 Compare August 7, 2023 21:17

nekeal and others added 7 commits August 7, 2023 23:19

Update the CHANGELOG.md

37bc2e5

Make use of get_option_at_index instead of directly accessing _options

22926cb

Move error handling for non-existing options to dedicated methods

a3590ac

Minor fixes

d63ac2c

Make use of get_option_index

913dce7

This simplifies error handling in a public interface

Add unit tests for multiline prompts

3ff5b7b

Add snapshot tests for replacing prompt in option list

def2628

nekeal force-pushed the feature/extend-optionlist branch from 978e0a3 to def2628 Compare August 7, 2023 21:19

willmcgugan approved these changes Aug 8, 2023

View reviewed changes

rodrigogiraoserrao merged commit 49612e3 into Textualize:main Aug 8, 2023
19 checks passed

	option = self._options[index]
	option = self.get_option_at_index(index)

Adds an interface for replacing prompt of an individual option in an OptionList #2985

Adds an interface for replacing prompt of an individual option in an OptionList #2985

Conversation

nekeal commented Jul 22, 2023

rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

BenMarkFrost Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nekeal Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

BenMarkFrost Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

nekeal Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

BenMarkFrost Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

nekeal Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigogiraoserrao commented Jul 22, 2023

willmcgugan left a comment

Choose a reason for hiding this comment

davep commented Jul 22, 2023

nekeal commented Jul 22, 2023 • edited Loading

willmcgugan commented Jul 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davep Jul 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigogiraoserrao commented Jul 23, 2023

nekeal commented Jul 23, 2023 • edited by rodrigogiraoserrao Loading

rodrigogiraoserrao commented Jul 25, 2023

nekeal commented Jul 25, 2023

rodrigogiraoserrao commented Jul 25, 2023

nekeal commented Jul 29, 2023

rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

rodrigogiraoserrao commented Aug 7, 2023

nekeal commented Aug 7, 2023

willmcgugan commented Aug 8, 2023

rodrigogiraoserrao commented Aug 8, 2023

davep commented Aug 8, 2023

Adds an interface for replacing prompt of an individual option in an `OptionList` #2985

Adds an interface for replacing prompt of an individual option in an `OptionList` #2985

BenMarkFrost Jul 22, 2023 •

edited

Loading

nekeal Jul 22, 2023 •

edited

Loading

BenMarkFrost Jul 22, 2023 •

edited

Loading

nekeal Jul 22, 2023 •

edited

Loading

BenMarkFrost Jul 22, 2023 •

edited

Loading

nekeal Jul 22, 2023 •

edited

Loading

nekeal commented Jul 22, 2023 •

edited

Loading

davep Jul 23, 2023 •

edited

Loading

nekeal commented Jul 23, 2023 •

edited by rodrigogiraoserrao

Loading