Add multiple choice prompt (map list type to questionary checkbox)

[tlambert03]

Closes #218 by supporting questionary.checkbox when the question type is list.

toppings:
    type: list
    choices:
        - Cheese
        - Tomatoes
        - Pepperoni
        - Sausage

This is a tiny PR at the moment... and it works fine for many cases. But feel free to point me to edge cases that you want caught and tested. (No need to guide my on the fixes yet, just state any concerns and I'll try to figure them out)

A potential elaboration would be to support list[<type>] with a more complicated cast_fn

For now, I am raising a validation error if type == 'list' but no choices are provided, which makes the validation logic much easier (since you'll always get a checkbox prompt from questionary). In the future, or here if you prefer, we can add logic to cast more general strings to lists

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.09%. Comparing base (42a34bf) to head (34ed685).
Report is 484 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1049      +/-   ##
==========================================
- Coverage   96.24%   96.09%   -0.16%     
==========================================
  Files          42       42              
  Lines        3169     3199      +30     
==========================================
+ Hits         3050     3074      +24     
- Misses        119      125       +6     

Flag	Coverage Δ
unittests	96.09% <100.00%> (-0.16%)	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pawamoy]

Does it work if we define choices as a YAML map instead of a list?

choices:
  key: value
  etc: etc

[tlambert03]

Does it work if we define choices as a YAML map instead of a list?

yes, test added. Also work with default

[yajo]

I'm not sure I like this interface.

Currently, when combining type and choices, type expresses the type of all choices. For example type: str tells Copier that no matter what choice is selected, it will be a string.

So I'd let that behavior as it is. For this example, we should have type: int.

Instead, I think it's better to add a new attribute multiselect: true which makes sense only when using choices. This way, the interface of this new option will be closer to current Copier behavior elsewhere.

WDYT?

[tlambert03]

Currently, when combining type and choices, type expresses the type of all choices. For example type: str tells Copier that no matter what choice is selected, it will be a string. So I'd let that behavior as it is. For this example, we should have type: int.

I'm not sure I agree here. Currently, with choices, you always get a single item back. So it makes sense to use the type (or union of types) that you will get back. Here, however, questionary will return you a list. So saying type: list isn't a hack, it's actually describing exactly what the type of the value will be. If you wanted to specify the type of each item, then I'd suggest using something like list[<type>] with a more complicated cast_fn as I mentioned in my original post.

I'm not opposed to the more explicit multi-select: true flag in addition though. But I do think you'd be setting things up for a bit of confusion if you leave type as the contained type, and users end up getting lists back. (in other words, I think type should always define the type of the value that you will end up with in your final context)

[yajo]

Hmm makes sense. Ok, let me play a bit with it 😁

[sisp]

I think this design decision is related to #750 and should take that feature proposal into consideration as well.

[sisp]

With #750 in mind, I disagree with

[...] it makes sense to use the type (or union of types) that you will get back.

because following that logic, a repeated question would have type: list[<type>] as well which would be ambiguous with multiple choice prompts, and a repeated multiple choice question would have type: list[list[<type>]].

Copier's questions configuration isn't a generic schema language like JSON Schema. It's a way to configure a question and not to describe an answer type or data structure. I think it's important to not confuse the two. So type isn't the type of the variable that contains the answer to that question but rather one config field among several whose meaning depends on the kind of question, which is defined in conjunction with the other fields.

[yajo]

We currently have yaml and json types which result in getting any kind of object/list/str/float/int.. anything supported by those formats.

I think it's confusing that you have type: list because Copier users have used lists since long ago by using those other 2 types.

Let's just make it clear that we're letting the user do a multi-select of choices, while still letting the type reflect the value contained within each of those choices. Just like it happens with single-select choices. Considering all options, let's use this implementation: #1049 (comment)

FWIW the default type is yaml IIRC. So we could have a {type: yaml, multiselect: true} question and that'd be just fine and readable. The docs should clarify usage anyways.

[brpaz]

@yajo I was experimenting a bit with this, before seeing this PR.

I was implementing like you said, to have a multiple option and keep the original prompt type - https://github.com/copier-org/copier/compare/master...brpaz:copier:multiselect?expand=1

The only issue I faced with this approach, is when rendering the jinja template, the value is passed as a string and not as a list, making any kind of operation like looping over all the choices difficult.

I managed to get a workaround, by setting the type as json. And maybe yaml also works as you said. I think it´s not very intuitive but I can live with that. :)

So, @tlambert03, any chances that you will do these changes on this PR? Otherwise, I can open a PR from my branch, if @yajo thinks it´s in the right direction

[tlambert03]

Go for it :)

[yajo]

The truth is that the type is a bit redundant when using select (or, now, multiselect) questions.

The only purpose of a type is to convert any user input to the expected type. But when you're only selecting options, it's the template designer who wrote the possible answers. Thus, it's fair to assume that it's their responsibility to be consistent on the chosen types.

Of course, this didn't suppose a problem when it was a single answer. But, if now, it's a problem, maybe a very simple approach would be to raise an error if you use multiple with a type that is not yaml or json. As usual, just add docs and go for the simplest solution :)

[tlambert03]

Thanks for the input all! Gonna close this and let @brpaz follow up