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

feat(choices): support questionary checkbox for multiple choices using multiple: true #1386

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

noirbizarre
Copy link
Contributor

@noirbizarre noirbizarre commented Oct 27, 2023

This PR is a sequel to #1049, taking into account review comments and #750 design proposal.

This PR only include the multiple: true form as the advanced form needs more specification and design I think, and it mostly relies on https://github.com/RhetTbull/questionary-superprompt#multiple-option which is not part of questionary.
However, it remains compatible as it is possible to add the advanced part later.

WDYT ?

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #1386 (7f97628) into master (9ea980f) will increase coverage by 0.03%.
Report is 3 commits behind head on master.
The diff coverage is 98.03%.

@@            Coverage Diff             @@
##           master    #1386      +/-   ##
==========================================
+ Coverage   97.27%   97.30%   +0.03%     
==========================================
  Files          48       48              
  Lines        4327     4423      +96     
==========================================
+ Hits         4209     4304      +95     
- Misses        118      119       +1     
Flag Coverage Δ
unittests 97.30% <98.03%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
copier/user_data.py 95.66% <100.00%> (+0.58%) ⬆️
tests/test_prompt.py 94.47% <97.70%> (+0.99%) ⬆️

@noirbizarre noirbizarre force-pushed the feature/multiple-choices branch 5 times, most recently from 0af288b to 86490ac Compare October 28, 2023 11:28
@noirbizarre
Copy link
Contributor Author

I don't get why coverage on tests/test_tools.py changed:

  • it's a test
  • not related to the feature
  • it has not been skipped

BTW, POE coverage commands differ from the CI one.

Why not this in CI ?:

poetry run poe coverage --cov-report=xml

This way it would be the same as locally with poe with only the xml coverage report in addition (the -ra is already in the pyproject.toml tool.pytest.ini_options.addopts setting) and tests would not be included in coverage by tests.

@noirbizarre
Copy link
Contributor Author

#1388 should fix the unexpected coverage change

@@ -173,6 +177,7 @@ class Question:
answers: AnswersMap
jinja_env: SandboxedEnvironment
choices: Union[Sequence[Any], Dict[Any, Any]] = field(default_factory=list)
multiple: bool = False
Copy link
Member

Choose a reason for hiding this comment

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

issue: the multiple word might seem OK as of today, but when #750 is implemented some day, it could become confusing.

This PR is about having the possibility to select multiple choices in a choices question. However, #750 is about asking the same question multiple times. In other words, the issue you're fixing is #218 instead.

Looking at https://github.com/RhetTbull/questionary-superprompt#multiple-option, it's possible that we use that solution when we implement #750. Thus, I'm afraid that using multiple now could be a shot in our own foot for the future.

So I think we should be able to rename this now, to help our future self.

WDYT about renaming this to multiselect?

Suggested change
multiple: bool = False
multiselect: bool = False

The same for other occurrences of this option.

FWIW, I can imagine a questionary where somebody wants to be able to make a multiselect choice multiple times (in the future, after this and #750 are implemented).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I am OK with this, I chose multiple as this was the term chosen in #750 (#750 define 2 behaviors, multiple: true and multiple: object) and #1049.

In fact, I thought a lot about it (that's what I was implying when saying "the advanced form needs more specification and design I think") because I think part of questionnary-superprompt might also be useful for multiselect: min/max make sense in multiselect as much as it make sense in multiple prompts. Poitn being that a possible evolution of multiselect: true is the same pattern than multiple: true: allowing to defined expected boundaries.

So multiselect it is, I'll update the PR accordingly 👍🏼 (thanks for pointing #218 because I missed it)

@yajo yajo linked an issue Oct 30, 2023 that may be closed by this pull request
@yajo yajo added this to the Soon milestone Oct 30, 2023
@noirbizarre noirbizarre force-pushed the feature/multiple-choices branch from 86490ac to 7f97628 Compare October 30, 2023 18:12
@noirbizarre
Copy link
Contributor Author

There it is, PR update for with multiselect.
I linked the commit to #218

Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Excellent, thanks!

@yajo yajo enabled auto-merge (rebase) October 30, 2023 18:19
@yajo yajo merged commit 99bdd11 into copier-org:master Oct 30, 2023
20 checks passed
@noirbizarre noirbizarre deleted the feature/multiple-choices branch October 30, 2023 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add multiple choice prompt
2 participants