-
-
Notifications
You must be signed in to change notification settings - Fork 240
Add numbered shortcuts and filter search to Question select/checkbox
#2446
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
base: master
Are you sure you want to change the base?
Conversation
…ilter_search` to `Question` class for selection with number shortcuts or filter Refs: copier-org#2155
…se_shortcuts` & `use_filter_search`
…& `use_filter_search`
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2446 +/- ##
==========================================
- Coverage 97.26% 97.20% -0.06%
==========================================
Files 55 56 +1
Lines 6288 6332 +44
==========================================
+ Hits 6116 6155 +39
- Misses 172 177 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sisp
left a comment
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.
Thanks for contributing these great enhancements of choice questions, @RR5555! 🙏
I have a few inline remarks. And could you please document these settings in the docs?
Per reviewer suggestion/request Reviewed-by: sisp Refs: copier-org#2446#discussion_r2678784231
Per reviewer suggestion/request Reviewed-by: sisp Refs: copier-org#2446#discussion_r2678784231
f043dd4 to
08371bf
Compare
…_filter_search` to `use_search_filter` Per reviewer catch to match `questionary` args as intended Reviewed-by: sisp Refs: copier-org#2446#discussion_r2678785014
Per reviewer request Reviewed-by: sisp Refs: copier-org#2446#discussion_r2678786515
Per reviewer suggestion/request Remove development leftovers Reviewed-by: sisp Refs: copier-org#2446#discussion_r2678777344 copier-org#2446#discussion_r2678779566
Per reviewer suggestion/request Reviewed-by: sisp Refs: copier-org#2446#discussion_r2678780560
…_filter` Per reviewer suggestion/request Reviewed-by: sisp Refs: copier-org#2446#pullrequestreview-3647014001
|
Thank you for reviewing another of my PRs ^^ I had a first go at the documentation of the settings (docs(docs/configuring.md): add docs for |
…se_searcg_filter` Per reviewer suggestion/request Reviewed-by: sisp Refs: copier-org#2446#discussion_r2678783429
…pes` change `dict` to `dict[str, Any]`
| Condition that, if `True`, will use `use_shortcuts` in `select` question, | ||
| allowing for selection via automatically numbered shortcut. Will be | ||
| deactivated if `use_search_filter` is `True`. |
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'd prefer describing only the behavior of the enabled feature and omitting a reference to the underlying implementation. Also, I'd prefer documenting mutual exclusiveness with multiselect (raising a validation error when combined) rather than silent deactivation.
| Condition that, if `True`, will use `use_shortcuts` in `select` question, | |
| allowing for selection via automatically numbered shortcut. Will be | |
| deactivated if `use_search_filter` is `True`. | |
| Condition that, if `True`, allows selecting choice question items via | |
| number shortcuts. Mutually exclusive with `multiselect`. |
| Condition that, if `True`, uses `use_search_filter` in `checkbox`/`select` | ||
| question while deactivating `use_jk_keys`, allowing for selection via | ||
| filtering. |
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'd prefer describing only the behavior of the enabled feature and omitting a reference to the underlying implementation.
| Condition that, if `True`, uses `use_search_filter` in `checkbox`/`select` | |
| question while deactivating `use_jk_keys`, allowing for selection via | |
| filtering. | |
| Condition that, if `True`, enables filtering choice question items by | |
| typing a search string. Disables j/k navigation, as "j" and "k" can be part | |
| of a prefix and therefore cannot be used for navigation. |
| - **multiselect**: When set to `true`, allows multiple choices. The answer will be a | ||
| `list[T]` instead of a `T` where `T` is of type `type`. | ||
|
|
||
| - **use_search_filter**: When set to `true`, . Also deactivates the use of `j`/`k` |
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.
The description of the effect of the feature is missing after the comma.
|
|
||
| !!! note | ||
|
|
||
| If `multiselect` is `true`, you cannot use `Space` in the search as this would actually just still select the option. If it is `false`, the `Space` character can be used in the search filter. |
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.
- If we enable the
pymdownx.keysextension, we'll be able to use pretty keyboard keys rendering in the documentation. WDYT? - Adding a comma before the "as" because it's used for a causal conjunction. 🤓
- Rephrasing "[...] actually just still [...]" a little. 🤓
- And just a bit more simplification. 🤓
| If `multiselect` is `true`, you cannot use `Space` in the search as this would actually just still select the option. If it is `false`, the `Space` character can be used in the search filter. | |
| If `multiselect` is `true`, you cannot use ++space++ in the search, as this | |
| would also select the choice item. If it is `false`, ++space++ can be used. |
|
|
||
| !!! note | ||
|
|
||
| If `use_shortcuts` & `use_search_filter` are both `true`, then only `use_search_filter` is activated. |
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'd remove this note and mention the mutual exclusiveness above.
| !!! note | |
| If `use_shortcuts` & `use_search_filter` are both `true`, then only `use_search_filter` is activated. |
| - python | ||
| - node | ||
| - c | ||
| - c++ | ||
| - rust | ||
| - zig | ||
| - asm | ||
| - a new language | ||
| - a good one | ||
| - an average one | ||
| - a not so good one |
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 stick with the same choices as in the example above? I think the list is already long enough with those choices, and typing o will filter python and node to show two remaining choices. I think the example showing the search filters an and ago isn't necessary – people will get the idea – while making the documentation quite lengthy.
|
|
||
| --- | ||
|
|
||
| You can use `Backspace` to modify the search filter. |
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.
If we enable the pymdownx.keys extension, we'll be able to use pretty keyboard keys rendering in the documentation. WDYT?
| You can use `Backspace` to modify the search filter. | |
| You can use ++backspace++ to modify the search filter. |
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.
It might just be me, but I find these parametrized tests a bit difficult to read and immediately understand what each case is testing exactly.
I was thinking about something like this – more repetitive but simple to read and grasp (IMHO):
from __future__ import annotations
import pexpect
import pytest
from .helpers import COPIER_PATH, Keyboard, Spawn, build_file_tree, expect_prompt
def test_shortcuts_disabled_by_default(
tmp_path_factory: pytest.TempPathFactory, spawn: Spawn
) -> None:
"""Shortcuts are disabled by default, so numbers don't select choices."""
src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
build_file_tree(
{
src / "copier.yml": (
"""\
select:
type: str
help: Select one option only
default: first
choices:
one: first
two: second
three: third
"""
),
src / "result.jinja": "{{ select }}",
}
)
tui = spawn(COPIER_PATH + ("copy", str(src), str(dst)))
expect_prompt(tui, "select", "str", help="Select one option only")
tui.send("3")
tui.send(Keyboard.Enter)
tui.expect_exact(pexpect.EOF)
assert (dst / "result").read_text() == "first"
def test_shortcuts_disabled(
tmp_path_factory: pytest.TempPathFactory, spawn: Spawn
) -> None:
"""When shortcuts are disabled, numbers don't select choices."""
src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
build_file_tree(
{
src / "copier.yml": (
"""\
select:
type: str
help: Select one option only
default: first
choices:
one: first
two: second
three: third
use_shortcuts: false
"""
),
src / "result.jinja": "{{ select }}",
}
)
tui = spawn(COPIER_PATH + ("copy", str(src), str(dst)))
expect_prompt(tui, "select", "str", help="Select one option only")
tui.send("3")
tui.send(Keyboard.Enter)
tui.expect_exact(pexpect.EOF)
assert (dst / "result").read_text() == "first"
def test_shortcuts_enabled(
tmp_path_factory: pytest.TempPathFactory, spawn: Spawn
) -> None:
"""When shortcuts are enabled, numbers select choices."""
src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
build_file_tree(
{
src / "copier.yml": (
"""\
select:
type: str
help: Select one option only
default: first
choices:
one: first
two: second
three: third
use_shortcuts: true
"""
),
src / "result.jinja": "{{ select }}",
}
)
tui = spawn(COPIER_PATH + ("copy", str(src), str(dst)))
expect_prompt(tui, "select", "str", help="Select one option only")
tui.send("3")
tui.send(Keyboard.Enter)
tui.expect_exact(pexpect.EOF)
assert (dst / "result").read_text() == "third"
@pytest.mark.skip( # TODO: Remove decorator when implemented
reason="Not implemented yet"
)
def test_multiselect_with_shortcuts_not_supported(
tmp_path_factory: pytest.TempPathFactory, spawn: Spawn
) -> None:
"""When shortcuts are enabled, numbers select choices."""
src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
build_file_tree(
{
src / "copier.yml": (
"""\
select:
type: str
help: Select one option only
multiselect: true
choices:
one: first
two: second
three: third
use_shortcuts: true
"""
)
}
)
tui = spawn(COPIER_PATH + ("copy", str(src), str(dst)))
# TODO: Check that an appropriate validation error is shown
def test_search_filter_disabled_by_default(
tmp_path_factory: pytest.TempPathFactory, spawn: Spawn
) -> None:
"""Search filter is disabled by default, so typing doesn't narrow choices."""
src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
build_file_tree(
{
src / "copier.yml": (
"""\
select:
type: str
help: Select one option only
default: first
choices:
one: first
two: second
three: third
"""
),
src / "result.jinja": "{{ select }}",
}
)
tui = spawn(COPIER_PATH + ("copy", str(src), str(dst)))
expect_prompt(tui, "select", "str", help="Select one option only")
tui.send("tw")
tui.send(Keyboard.Enter)
tui.expect_exact(pexpect.EOF)
assert (dst / "result").read_text() == "first"
def test_search_filter_disabled(
tmp_path_factory: pytest.TempPathFactory, spawn: Spawn
) -> None:
"""When search filter is disabled, typing doesn't narrow choices."""
src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
build_file_tree(
{
src / "copier.yml": (
"""\
select:
type: str
help: Select one option only
default: first
choices:
one: first
two: second
three: third
use_search_filter: false
"""
),
src / "result.jinja": "{{ select }}",
}
)
tui = spawn(COPIER_PATH + ("copy", str(src), str(dst)))
expect_prompt(tui, "select", "str", help="Select one option only")
tui.send("tw")
tui.send(Keyboard.Enter)
tui.expect_exact(pexpect.EOF)
assert (dst / "result").read_text() == "first"
def test_search_filter_enabled(
tmp_path_factory: pytest.TempPathFactory, spawn: Spawn
) -> None:
"""When search filter is enabled, typing narrows choices to matching options."""
src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
build_file_tree(
{
src / "copier.yml": (
"""\
select:
type: str
help: Select one option only
default: first
choices:
one: first
two: second
three: third
use_search_filter: true
"""
),
src / "result.jinja": "{{ select }}",
}
)
tui = spawn(COPIER_PATH + ("copy", str(src), str(dst)))
expect_prompt(tui, "select", "str", help="Select one option only")
tui.send("tw")
tui.send(Keyboard.Enter)
tui.expect_exact(pexpect.EOF)
assert (dst / "result").read_text() == "second"
def test_search_filter_and_shortcut_with_number_input(
tmp_path_factory: pytest.TempPathFactory, spawn: Spawn
) -> None:
"""
When search filter and shortcuts are enabled, numbers filter choices rather than
selecting them.
"""
src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
build_file_tree(
{
src / "copier.yml": (
"""\
select:
type: str
help: Select one option only
default: first
choices:
one: first
two: second
three: third
four: forth
use_search_filter: true
use_shortcuts: true
"""
),
src / "result.jinja": "{{ select }}",
}
)
tui = spawn(COPIER_PATH + ("copy", str(src), str(dst)))
expect_prompt(tui, "select", "str", help="Select one option only")
tui.send("3")
tui.send(Keyboard.Enter)
tui.expect_exact(pexpect.EOF)
assert (dst / "result").read_text() == "first"
def test_multiselect_with_search_filter(
tmp_path_factory: pytest.TempPathFactory, spawn: Spawn
) -> None:
"""Search filter works with multiselect prompts."""
src, dst = map(tmp_path_factory.mktemp, ("src", "dst"))
build_file_tree(
{
src / "copier.yml": (
"""\
checkbox:
type: str
help: Select any option
multiselect: true
choices:
one: first
two: second
three: third
use_search_filter: true
"""
),
src / "result.jinja": "{{ checkbox }}",
}
)
tui = spawn(COPIER_PATH + ("copy", str(src), str(dst)))
expect_prompt(tui, "checkbox", "str", help="Select any option")
tui.send("tw ")
tui.send(Keyboard.Enter)
tui.expect_exact(pexpect.EOF)
assert (dst / "result").read_text() == "['second']"The test function test_multiselect_with_shortcuts_not_supported is currently skipped because
- Copier currently doesn't raise a validation error when
multiselectanduse_shortcutsare combined, and - the check for this validation error is still missing in the test function.
WDYT?
|
|
||
|
|
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.
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.
Kudos for the beautiful prompt visualization using HTML in Markdown including the Unicode icon for the question type! 🏆 This looks great! 🤩
PR Description
This PR adds the
questionaryoptionsuse_shortcuts&use_filter_searchto CopierQuestion, to allow the usage of numbered shortcuts forselectquestions, and filter search forselect/checkboxquestions.It addresses Issue #2155
It adds two new attributes to the
Questionclass:use_shortcuts: booluse_filter_search: boolWhich are passed down to
questionary, thus producing the desired effect.Note
use_filter_searchis not compatible withuse_jk_keysquestionaryoption which is activated by default. Thus, whenuse_filter_searchisTruefor aselect/checkboxquestion, we pass and putuse_jk_keystoFalse.Choice
Although, it is possible to pass both
{use_filter_search: True, use_shortcuts: True}forselectquestions, I have decided to makeuse_filter_searchhave precedence overuse_shortcutsto avoid any ambiguity. Thus, if both areTruefor aselectquestion, onlyuse_filter_searchis passed and activated.Usage
use_shortcutscopier.yamlThis will display:
And pressing
3will direct the cursor onto3) three.use_filter_searchcopier.yamlPressing
twill make a field appeared at the bottom left with/ t...while only showing:Next, pressing
wwill change the bottom field to/ tw..while only showing:Pressing Backspace will delete the searched letter one at a time, allowing for correction or new/additional filter search.
Limitation
The two new attributes are only booleans, they are not
str|bool, and not rendered.