Adding descriptions for each option

[iamalisalehi]

Hi,

This patch provides a feature to show descriptions for each options. The description for each option shows on the right side of the screen as the cursor moves on to it. A function was also added that divides long description into multiple lines based on the terminal size. An example was also provided.

Hope it is useful.

P.S. I couldn't think of a way to write a test for it so I didn't.

[iamalisalehi]

Here's a gif of the example:

[SnowzNZ]

I feel like it would be better to have descriptions and options stored as a dictionary rather than two lists.

[gustavo-mag]

unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AUX7SDUCK4CSA6RXM6LLD4LY2MRPPAVCNFSM6AAAAABFJWZTKGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRTHA3TIMRWGY__;!!JVh_Qfuk4otXm3Mt0g!67AzWURm-WTfVyJpsuZ6U4uEqcwCB4llajiC0go_pxsS-Rk54x5KkK2GxpWOwPz4UXEQAqN5UFxxxyIcx71tsnPXNSO_Aakh$

Internal Use Only

From: Snowz @.> Sent: quarta-feira, 27 de março de 2024 17:09 To: wong2/pick @.> Cc: Subscribed @.***> Subject: Re: [wong2/pick] Adding descriptions for each option (PR #114)

CAUTION: External email.

I feel like it would be better to have descriptions and options stored as a dictionary rather than two lists.

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/wong2/pick/pull/114*issuecomment-2023874266__;Iw!!JVh_Qfuk4otXm3Mt0g!67AzWURm-WTfVyJpsuZ6U4uEqcwCB4llajiC0go_pxsS-Rk54x5KkK2GxpWOwPz4UXEQAqN5UFxxxyIcx71tsnPXNVZB2ZlH$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AUX7SDUCK4CSA6RXM6LLD4LY2MRPPAVCNFSM6AAAAABFJWZTKGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRTHA3TIMRWGY__;!!JVh_Qfuk4otXm3Mt0g!67AzWURm-WTfVyJpsuZ6U4uEqcwCB4llajiC0go_pxsS-Rk54x5KkK2GxpWOwPz4UXEQAqN5UFxxxyIcx71tsnPXNSO_Aakh$. You are receiving this because you are subscribed to this thread.Message ID: @.**@.>>

[iamalisalehi]

I feel like it would be better to have descriptions and options stored as a dictionary rather than two lists.

That's a nice idea. I'll try it.

[iamalisalehi]

So I just added a new commit which incorporates @SnowzNZ's idea with minimal changes to the way the code is written to still be backwards compatible (Although the descriptions will only work properly with python3.7+ because the dictionaries weren't ordered before that).

please read the commit notes too.

[wong2]

In case you don't know, the item passed to options could be an Option instance, so I think it's better to add description as an attribute of Option.

https://github.com/wong2/pick/blob/master/src/pick/__init__.py#L22 https://github.com/wong2/pick/blob/master/example/option.py

[aisk]

Hi @iamalisalehi, if you want to continue the work, please merge the latest code from the master branch, which has fixed the CI, and we should ensure that it passes before merging.

[iamalisalehi]

Hi @wong2 , thanks for the review. Sorry I am not still very skilled in git and messed up commit messages a little bit so I'll explain what I did here:

1) Added an optional attribute description to the Option class. 2) Changed parse_options function to turn dictionaries in instances of Option. Now we don't need self.descriptions anymore. 3) Added a new example. 4) Updated test_option to include the new possibilities. 5) Changed the test for parsing dictionaries.

P.S. I didn't change value attribute, only gave it a default None value. Is this okay?

[iamalisalehi]

hi @aisk , so I'm seeing that type checks failed for python<3.10. The thing is that I used mypy cheatsheet to rewrite the type hints for python>=3.10: For example: typing.List to list and so on. Should I revert it back?

[aisk]

Yes, since we support old version of Python, we have to use some old grammars for type hints.

[SnowzNZ]

Lecture

?

[aisk]

Actually, the current codes don't have the expected behavior on my machine. I guess it's been broken during the updated commits. This is the result of running python example/description_dict.py:

[aisk]

When picking a dict, the pick result is an Option. But in the current main branch, you pass a list of str of Option, and the pick result will be the original type.

So I think the pick result of a dict should be a two-element tuple, like when iterating over a dict.

[iamalisalehi]

Actually, the current codes don't have the expected behavior on my machine. I guess it's been broken during the updated commits. This is the result of running python example/description_dict.py:

Hi @aisk , I fixed the issues you raised, and checked the example again. It worked well on my computer (I am running python3.11.8). Can you please check it again?

[iamalisalehi]

When picking a dict, the pick result is an Option. But in the current main branch, you pass a list of str of Option, and the pick result will be the original type.

So I think the pick result of a dict should be a two-element tuple, like when iterating over a dict.

Hmm, I thought about this but I am not so sure that it would be the best way to do it. As I understood from the code before I worked on it, there is a simple way and a more complex way to use it. In the first versions that I wrote, adding descriptions was considered a part of the simple way of doing things but then at the suggestion of @wong2 , I changed it to be a part of the complex way of doing it (this also makes more sense to me). But I feel like that returning a tuple, introduces a third way of doing things and needs another datatype to be considered and might introduce unnecessary complexity. But as you have probably seen, I don't have a lot of coding experience so I am not sure..

[aisk]

Hi @aisk , I fixed the issues you raised, and checked the example again. It worked well on my computer (I am running python3.11.8). Can you please check it again?

It's the same result, but I'm using Linux with Tilix as terminal emulator. Maybe this is OS dependent issue, I'll check the result in other OS tomorrow.

---

Update: Kitty on Linux have no issue:

[iamalisalehi]

Hi @aisk , I fixed the issues you raised, and checked the example again. It worked well on my computer (I am running python3.11.8). Can you please check it again?

It's the same result, but I'm using Linux with Tilix as terminal emulator. Maybe this is OS dependent issue, I'll check the result in other OS tomorrow.

Update: Kitty on Linux have no issue:

Does the option.py example work on Tilix or is it just the dict example?

P.S. I tested them on Konsole, Terminator and Pycharm's terminal and they work

[aisk]

When picking a dict, the pick result is an Option. But in the current main branch, you pass a list of str of Option, and the pick result will be the original type. So I think the pick result of a dict should be a two-element tuple, like when iterating over a dict.

Hmm, I thought about this but I am not so sure that it would be the best way to do it. As I understood from the code before I worked on it, there is a simple way and a more complex way to use it. In the first versions that I wrote, adding descriptions was considered a part of the simple way of doing things but then at the suggestion of @wong2 , I changed it to be a part of the complex way of doing it (this also makes more sense to me). But I feel like that returning a tuple, introduces a third way of doing things and needs another datatype to be considered and might introduce unnecessary complexity. But as you have probably seen, I don't have a lot of coding experience so I am not sure..

I have some slight concerns about the complexity too, so I think for now, maybe we should only support using list[pick.Option] as the input type for items with descriptions.

In this way, we can ensure that the pick's result type matches the input type exactly. If you put a list of str, you'll get a str; if you put a list of Option, you'll get an Option.

[iamalisalehi]

I have some slight concerns about the complexity too, so I think for now, maybe we should only support using list[pick.Option] as the input type for items with descriptions.

So I should get rid of the parse_options function completely?

In this way, we can ensure that the pick's result type matches the input type exactly. If you put a list of str, you'll get a str; if you put a list of Option, you'll get an Option.

Question: In the option example now we see both str and Option inputs. Should this be acceptable? It outputs exactly the same thing and yet the input is not uniform.

[aisk]

So I should get rid of the parse_options function completely?

Yes, and thus we can revert specify the self.options to Any.

Question: In the option example now we see both str and Option inputs. Should this be acceptable? It outputs exactly the same thing and yet the input is not uniform.

You reminded me, I think it's not accepted, because I guess mypy or pyright will complain about this, I'm not using my development machine so I don't try it, but you can have a try. I think we should add a type checker to the examples on the GHA.

[iamalisalehi]

So I should get rid of the parse_options function completely?

Yes, and thus we can revert specify the self.options to Any.

I did that, and also updated the tests.

I think it's not accepted, because I guess mypy or pyright will complain about this.

mypy did complain so I had to only feed it Options or strs at a time. Fixed the option.py example and removed dict example.

I think we should add a type checker to the examples on the GHA.

I am a bit confused, is it something different than what mypy checks already do?

[aisk]

LGTM. I'll wait a few days before merge to give @wong2 a chance to take a look if he has time. Thanks for your contribution! @iamalisalehi

[iamalisalehi]

Thank you for the great reviews @aisk . I learned a lot from you ;-)