Adding descriptions for each option #114

iamalisalehi · 2024-03-26T21:21:07Z

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.

Added the option to show description messages for each option by moving the cursor on to them.

Fixed some screen size math

iamalisalehi · 2024-03-26T21:21:52Z

Here's a gif of the example:

SnowzNZ · 2024-03-27T20:09:04Z

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

gustavo-mag · 2024-03-27T20:48:14Z

unsubscribe<https://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 GitHub<https://urldefense.com/v3/__https:/github.com/wong2/pick/pull/114*issuecomment-2023874266__;Iw!!JVh_Qfuk4otXm3Mt0g!67AzWURm-WTfVyJpsuZ6U4uEqcwCB4llajiC0go_pxsS-Rk54x5KkK2GxpWOwPz4UXEQAqN5UFxxxyIcx71tsnPXNVZB2ZlH$>, or unsubscribe<https://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 · 2024-03-27T23:12:37Z

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.

…scriptions. 2) changed the example to work with the new way of using the code. 3) wrote a new test for descriptions

iamalisalehi · 2024-03-28T16:10:26Z

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.

SnowzNZ

Remove .idea folder

wong2 · 2024-03-31T09:40:38Z

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

example/description.py

aisk · 2024-04-01T11:15:14Z

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 · 2024-04-01T15:57:47Z

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:

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

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

aisk

There are some comments for the PR, and the CI should be passed, I see it failed mainly because the type checker, you can run mypy on your local machine to check and then fix them.

example/description_option.py

src/pick/__init__.py

example/description_dict.py

src/pick/__init__.py

tests/test_pick.py

iamalisalehi · 2024-04-02T14:35:54Z

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 · 2024-04-02T14:41:47Z

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

src/pick/__init__.py

aisk · 2024-04-03T08:37:21Z

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 · 2024-04-03T08:40:42Z

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 · 2024-04-03T13:53:52Z

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 · 2024-04-03T14:04:30Z

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 · 2024-04-03T14:10:06Z

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 · 2024-04-03T17:34:29Z

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

I resolved the problem; it's not caused by Tilix, but by a wider screen. With a wider screen, the last value passed to addstr in draw method will be larger than 256, which will draw the description text with the same color as the background, leading to invisibility.

Kitty works fine on my machine because the default font is larger than Tilix's, so the screen is not wide enoght to triger the bug.

I guess you want to use addnstr instead of addstr to specify the maximum line width, but you missed an n, which caused the bug.

src/pick/__init__.py

Co-authored-by: AN Long <aisk@users.noreply.github.com>

aisk · 2024-04-06T14:44:12Z

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 · 2024-04-06T17:15:07Z

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 · 2024-04-07T14:15:40Z

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 · 2024-04-07T16:43:14Z

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?

src/pick/__init__.py

Co-authored-by: AN Long <aisk@users.noreply.github.com>

aisk · 2024-04-08T10:54:04Z

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 · 2024-04-08T10:56:31Z

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

iamalisalehi added 3 commits March 27, 2024 00:11

Update __init__.py

4899b48

Added the option to show description messages for each option by moving the cursor on to them.

An example to show descriptions.

912da47

Update __init__.py

82a75ea

Fixed some screen size math

1) changed the main program to parse dictionaries for options with de…

71ab424

…scriptions. 2) changed the example to work with the new way of using the code. 3) wrote a new test for descriptions

SnowzNZ suggested changes Mar 30, 2024

View reviewed changes

Delete .idea directory

58dc2e7

aisk reviewed Apr 1, 2024

View reviewed changes

example/description.py Outdated Show resolved Hide resolved

iamalisalehi and others added 4 commits April 1, 2024 17:05

Merge branch 'master' into master

d05493f

added optional attribute to the class

678c7c0

add to

2819f15

trying to fix some commit messages

1103de8

Delete .idea directory

0fbcfdf

aisk reviewed Apr 2, 2024

View reviewed changes

example/description_option.py Outdated Show resolved Hide resolved

src/pick/__init__.py Outdated Show resolved Hide resolved

example/description_dict.py Outdated Show resolved Hide resolved

src/pick/__init__.py Outdated Show resolved Hide resolved

tests/test_pick.py Show resolved Hide resolved

iamalisalehi added 3 commits April 2, 2024 17:52

Fixed types and refactored examples

95acccb

changed test_option()

cffdf81

fixed type checks for the eddited test

187f047

iamalisalehi added 2 commits April 2, 2024 18:23

revert back type hints grammer

f327cc3

fixed an error caused ny find/replace all

d501f20

This comment was marked as off-topic.

Sign in to view

aisk reviewed Apr 3, 2024

View reviewed changes

src/pick/__init__.py Outdated Show resolved Hide resolved

src/pick/__init__.py Outdated Show resolved Hide resolved

Applied early return principle and fixed some minor stuff

388969c

aisk reviewed Apr 4, 2024

View reviewed changes

src/pick/__init__.py Outdated Show resolved Hide resolved

Resolving widescreen bug

9098b2c

Co-authored-by: AN Long <aisk@users.noreply.github.com>

iamalisalehi added 4 commits April 7, 2024 19:58

revert back support for dictionary input

fb98b9f

update examples

9d085dc

update tests

cb4b44a

revert List to Sequence for options type

b946530

aisk reviewed Apr 8, 2024

View reviewed changes

src/pick/__init__.py Outdated Show resolved Hide resolved

src/pick/__init__.py Outdated Show resolved Hide resolved

iamalisalehi and others added 2 commits April 8, 2024 12:36

Update src/pick/__init__.py

be993c5

Co-authored-by: AN Long <aisk@users.noreply.github.com>

If no option present, use the whole line

87540ac

aisk approved these changes Apr 8, 2024

View reviewed changes

aisk merged commit d1a8e6b into aisk:master Apr 11, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding descriptions for each option #114

Adding descriptions for each option #114

iamalisalehi commented Mar 26, 2024

iamalisalehi commented Mar 26, 2024

SnowzNZ commented Mar 27, 2024

gustavo-mag commented Mar 27, 2024 via email

iamalisalehi commented Mar 27, 2024

iamalisalehi commented Mar 28, 2024

SnowzNZ left a comment

wong2 commented Mar 31, 2024 •

edited

Loading

aisk commented Apr 1, 2024

iamalisalehi commented Apr 1, 2024

aisk left a comment •

edited

Loading

iamalisalehi commented Apr 2, 2024

aisk commented Apr 2, 2024

This comment was marked as off-topic.

aisk commented Apr 3, 2024 •

edited

Loading

aisk commented Apr 3, 2024

iamalisalehi commented Apr 3, 2024

iamalisalehi commented Apr 3, 2024

aisk commented Apr 3, 2024 •

edited

Loading

iamalisalehi commented Apr 3, 2024

aisk left a comment •

edited

Loading

aisk commented Apr 6, 2024

iamalisalehi commented Apr 6, 2024

aisk commented Apr 7, 2024 •

edited

Loading

iamalisalehi commented Apr 7, 2024

aisk commented Apr 8, 2024

iamalisalehi commented Apr 8, 2024 •

edited

Loading

Adding descriptions for each option #114

Adding descriptions for each option #114

Conversation

iamalisalehi commented Mar 26, 2024

iamalisalehi commented Mar 26, 2024

SnowzNZ commented Mar 27, 2024

gustavo-mag commented Mar 27, 2024 via email

iamalisalehi commented Mar 27, 2024

iamalisalehi commented Mar 28, 2024

SnowzNZ left a comment

Choose a reason for hiding this comment

wong2 commented Mar 31, 2024 • edited Loading

aisk commented Apr 1, 2024

iamalisalehi commented Apr 1, 2024

aisk left a comment • edited Loading

Choose a reason for hiding this comment

iamalisalehi commented Apr 2, 2024

aisk commented Apr 2, 2024

This comment was marked as off-topic.

aisk commented Apr 3, 2024 • edited Loading

aisk commented Apr 3, 2024

iamalisalehi commented Apr 3, 2024

iamalisalehi commented Apr 3, 2024

aisk commented Apr 3, 2024 • edited Loading

iamalisalehi commented Apr 3, 2024

aisk left a comment • edited Loading

Choose a reason for hiding this comment

aisk commented Apr 6, 2024

iamalisalehi commented Apr 6, 2024

aisk commented Apr 7, 2024 • edited Loading

iamalisalehi commented Apr 7, 2024

aisk commented Apr 8, 2024

iamalisalehi commented Apr 8, 2024 • edited Loading

wong2 commented Mar 31, 2024 •

edited

Loading

aisk left a comment •

edited

Loading

aisk commented Apr 3, 2024 •

edited

Loading

aisk commented Apr 3, 2024 •

edited

Loading

aisk left a comment •

edited

Loading

aisk commented Apr 7, 2024 •

edited

Loading

iamalisalehi commented Apr 8, 2024 •

edited

Loading