Skip to content
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

Add excluded indexes. #105

Closed
wants to merge 12 commits into from
Closed

Add excluded indexes. #105

wants to merge 12 commits into from

Conversation

J3ldo
Copy link
Contributor

@J3ldo J3ldo commented May 14, 2023

Added excluded indexes, excluded indexes are put in as a list and will be skipper over when tried to select.
An user may need this for file selection to make the directories unselectable.

Code added:

while self.index in self.excluded_indexes:
    self.index -= 1  # - for move_down, and + for move_up

@J3ldo
Copy link
Contributor Author

J3ldo commented May 18, 2023

Also in issue #29

@wong2
Copy link
Collaborator

wong2 commented Mar 31, 2024

I think it's better to add it as an attribute of Option

@@ -56,6 +56,7 @@ interactive selection list in the terminal.
multiple items by hitting SPACE
- `min_selection_count`: (optional) for multi select feature to
dictate a minimum of selected items before continuing
- `excluded_indexes`: (optional) indexes excluded from the selector. Excluded indexes will make it that they wont be able to get selected and just skip over them
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `excluded_indexes`: (optional) indexes excluded from the selector. Excluded indexes will make it that they wont be able to get selected and just skip over them

@@ -32,6 +32,7 @@ class Picker(Generic[OPTION_T]):
multiselect: bool = False
min_selection_count: int = 0
selected_indexes: List[int] = field(init=False, default_factory=list)
excluded_indexes: List[int] = field(default_factory=list)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
excluded_indexes: List[int] = field(default_factory=list)

Comment on lines +52 to +53
while self.index in self.excluded_indexes:
self.index += 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
while self.index in self.excluded_indexes:
self.index += 1



Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +58 to 62

while self.index in self.excluded_indexes:
self.index -= 1
if self.index < 0:
self.index = len(self.options) - 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
while self.index in self.excluded_indexes:
self.index -= 1
if self.index < 0:
self.index = len(self.options) - 1
if self.index < 0:
self.index = len(self.options) - 1
if isinstance(self.options[self.index], Option) and not self.options[self.index].enable:
self.index -= 1

Comment on lines +66 to 70

while self.index in self.excluded_indexes:
self.index += 1
if self.index >= len(self.options):
self.index = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
while self.index in self.excluded_indexes:
self.index += 1
if self.index >= len(self.options):
self.index = 0
if self.index >= len(self.options):
self.index = 0
if isinstance(self.options[self.index], Option) and not self.options[self.index].enable:
self.index += 1

@@ -187,15 +197,20 @@ def pick(
default_index: int = 0,
multiselect: bool = False,
min_selection_count: int = 0,
excluded_indexes = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
excluded_indexes = None,

Comment on lines +203 to +205
if excluded_indexes is None:
excluded_indexes = []

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if excluded_indexes is None:
excluded_indexes = []

picker: Picker = Picker(
options,
title,
indicator,
default_index,
multiselect,
min_selection_count,
excluded_indexes,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
excluded_indexes,

Copy link
Contributor

@iamalisalehi iamalisalehi Apr 13, 2024

Choose a reason for hiding this comment

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

To implement @wong2 's suggestion, you can add an extra attribute to the class Option, something like this:

enable: bool = True

Then, if you implement my suggestions, everything should work as intended

@iamalisalehi
Copy link
Contributor

Hi @aisk , It seems like the person who originally opened this PR, isn't interested anymore. Are you planning on fixing and merging it?

@aisk
Copy link
Owner

aisk commented Jul 10, 2024

@iamalisalehi This change looks almost good to me with what you pointed out. But some work can't be done just by clicking the submit button in your reviews above, like adding a field to the Options class.

So I think we need to create a new PR to continue the work. And if you are interested in it, you can just do it, but one thing to note is that, if you want to use the code in this PR, it should keep the commits in this PR, or add the original author to the co-author field in commit messages, to appreciate their contributions.

@iamalisalehi iamalisalehi mentioned this pull request Jul 11, 2024
@aisk aisk closed this Jul 28, 2024
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.

4 participants