fix: require answer for questions without default value

[sisp]

I've fixed the behavior for questions without a default value when no answer is provided in the interactive questionnaire.

The new behavior is:

• type: str question: ""
• type: yaml question: None (result of yaml.safe_load(""))
• type: int/type: float/type: json question: Validation error Invalid input
• type: bool question: False (implicit default)
• choice: ... question: The first choice (implicit default).

I think this new behavior is more intuitive and expected than the current one.

Fixes #355 #956.

Note: This PR does not change the behavior when the --default flag is passed because it is not yet clear how to deal with questions that lead to a validation error when no default value exists.

[codecov]

Codecov Report

Merging #958 (bdf84ad) into master (42a34bf) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #958      +/-   ##
==========================================
+ Coverage   96.24%   96.34%   +0.10%     
==========================================
  Files          42       44       +2     
  Lines        3169     3448     +279     
==========================================
+ Hits         3050     3322     +272     
- Misses        119      126       +7     

Flag	Coverage Δ
unittests	96.34% <100.00%> (+0.10%)	⬆️

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

Impacted Files	Coverage Δ
tests/test_answersfile.py	100.00% <ø> (ø)
copier/main.py	94.79% <100.00%> (+0.11%)	⬆️
copier/types.py	86.00% <100.00%> (+0.58%)	⬆️
copier/user_data.py	94.59% <100.00%> (-0.34%)	⬇️
tests/test_copy.py	100.00% <100.00%> (ø)
tests/test_prompt.py	91.34% <100.00%> (+1.69%)	⬆️
tests/test_templated_prompt.py	100.00% <100.00%> (ø)

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[yajo]

For int and float, the default could be 0. And for Json, null. If we do it like that, we avoid the errors. What do you think?

[sisp]

I think using 0 as a default value for int and float is not safe, especially when the validator field is set with a check that disallows this value. And without the validator field, it's still impossible to know whether 0 is a valid value in a given questionnaire, and for that reason the validator field should exist for all constrained questions, but it's still a quite new feature and might not have been adopted yet.

After giving this some more thought and reading #355 (comment) again, I think whether the correct/consistent solution is this:

The --defaults flag (or equivalent argument via API) can only be used when the default field is set for all questions. Otherwise, an error is raised. Questions without a default field are required and must be answered either interactively or via the --data flag (or equivalent argument via API).

In interactive mode, there are some implicit initial values/selections:

• str question: "" because this is the best guess initial value for the input prompt.
• bool question: False because questionary's confirm question requires a default value and False seems better than True IMO.
• choice: ... question: "The first choice" because questionary doesn't support no initial choice and falls back to the first choice if no default value is provided.

For int/float/yaml/json questions, I'd initialize the input prompt empty and require a valid answer - also for yaml where I'd not treat an empty or spaces-only string as null although yaml.safe_load("") behaves like that because it is inconsistent with json and not intuitive IMO. If somebody wants to answer with null, then they should answer null.

That said, I don't agree with

[...] but an empty string in the default field should not imply required=true since in some cases the default value may be an empty string.

— #355 (comment)

because default: "" is a perfectly fine value or a str question and I see no reason why it should ever imply required: true then. In fact, the presence of a default field implies an optional question and the absence of a default field implies a required field, so having both a default field and a required field is redundant.

WDYT?

[yajo]

The --defaults flag (or equivalent argument via API) can only be used when the default field is set for all questions. Otherwise, an error is raised. Questions without a default field are required and must be answered either interactively or via the --data flag (or equivalent argument via API).

So, --defaults will work as long as all questions without defaults are answered via --data, right?

because default: "" is a perfectly fine value or a str question and I see no reason why it should ever imply required: true then. In fact, the presence of a default field implies an optional question and the absence of a default field implies a required field, so having both a default field and a required field is redundant.

I agree. For a required str question, just add `validator: '{% if not answer %}Required{% endif %}'. It's not as ergonomic, but works and is simpler to maintain upstream.

[yajo]

for yaml where I'd not treat an empty or spaces-only string as null although yaml.safe_load("") behaves like that because it is inconsistent with json and not intuitive IMO. If somebody wants to answer with null, then they should answer null.

This behavior is documented in the YAML spec, so I think we should respect the spec and let empty strings become None when parsed.

[sisp]

So, --defaults will work as long as all questions without defaults are answered via --data, right?

Correct.

This behavior is documented in the YAML spec, so I think we should respect the spec and let empty strings become None when parsed.

I hadn't thought about that. Right, the spec should certainly be respected. 👍

[sisp]

I've made the changes as we discussed. Any question that has no default field is considered to be a required question and needs an explicit answer via interactive mode, CLI or API. When the --defaults flag is used, questions without a default field need to be answered via the --data flag. Note that --defaults implies non-interactive mode, i.e. it is not possible to interactively answer questions without a default field. This behavior seems consistent with the current behavior as --defaults for questions without a default field result in the answer None. If we want to distinguish between interactive and non-interactive mode, we might need to add another flag in a separate PR.

The implementation of this behavior requires a new value that has the semantics "not set" for the value of the default field when the default field is missing. None doesn't work because it might be a regular default value. Therefore, I introduced a sentinel type MissingType and a corresponding constant MISSING inspired by python/mypy#5424 (comment).

A few tests require adjustment to the new behavior because they rely on the default answer to a question without a default field to be None. I've made those changes.

WDYT, @yajo?

[yajo]

Great!

[yajo]

Would you please fix the conflics to merge?

[sisp]

Done. And I've also added type hints and made some minor stylistic improvements.

[sisp]

I just pushed a commit that removes obsolete special treatment of str questions when the answer is None because this isn't valid anymore.