Improved check for choices and default value

[phospi]

Hi,

I created this PR because I am observing that the following options-documenation is parsed with an error:

options:
  add_ins:
    description:
    - Contains list of addIns
    - see addIn documentation - L(here,https://website)
    default:
    elements: dict
    type: list

The error message looks like this:

Traceback (most recent call last):
  File "/root/.local/share/virtualenvs/spirit21.general-ZSKsEIAg/bin/collection_prep_add_docs", line 8, in <module>
    sys.exit(main())
  File "/root/.local/share/virtualenvs/spirit21.general-ZSKsEIAg/lib/python3.8/site-packages/collection_prep/cmd/add_docs.py", line 548, in main
    content = process(collection=collection, path=path)
  File "/root/.local/share/virtualenvs/spirit21.general-ZSKsEIAg/lib/python3.8/site-packages/collection_prep/cmd/add_docs.py", line 385, in process
    fd.write(template.render(doc))
  File "/root/.local/share/virtualenvs/spirit21.general-ZSKsEIAg/lib/python3.8/site-packages/jinja2/environment.py", line 1289, in render
    self.environment.handle_exception()
  File "/root/.local/share/virtualenvs/spirit21.general-ZSKsEIAg/lib/python3.8/site-packages/jinja2/environment.py", line 924, in handle_exception
    raise rewrite_traceback_stack(source=source)
  File "/root/.local/share/virtualenvs/spirit21.general-ZSKsEIAg/lib/python3.8/site-packages/collection_prep/cmd/plugin.rst.j2", line 102, in top-level template code
    {% for key, value in options|dictsort recursive %}
  File "/root/.local/share/virtualenvs/spirit21.general-ZSKsEIAg/lib/python3.8/site-packages/collection_prep/cmd/plugin.rst.j2", line 156, in template
    {% if value.default is defined and value.default not in value.choices %}

Solution Discussion

Now I see a lot of room for discussion about different approaches to the issue:

1. Is the above documentation example a correct documentation?
2. Change the documentation
3. Add choices to documentation

1. Is the above documentation example a correct documentation?

I don't know for sure. I read the Guide here and there is no imperative to leave default out, if there are no choices. However, between the lines you get the idea that if values are empty, you should leave out the keys as well. However, the jinja template fails as well for me if a default value is given, e.g. default: Test. Because there is still no choices key.

2. Change the documentation

Obviously we can remove default: but we have no obligation to do so. If that should be the solution then we should catch the error and inform the user.

3. Add choices to documentation

The below documentation example works without error.

options:
  add_ins:
    description:
    - Contains list of addIns
    - see addIn documentation - L(here,https://website)
    default:
    choices: []
    elements: dict
    type: list

[gravesm]

The problem you are seeing here is because of a breaking change in Jinja2 3.0.0. I would suggest instead of your current solution the following:

diff --git a/collection_prep/cmd/plugin.rst.j2 b/collection_prep/cmd/plugin.rst.j2
index 5cf357a..d2271ad 100644
--- a/collection_prep/cmd/plugin.rst.j2
+++ b/collection_prep/cmd/plugin.rst.j2
@@ -153,7 +153,7 @@ Parameters
                         </ul>
                     {% endif %}
                     {# Show default value, when multiple choice or no choices #}
-                    {% if value.default is defined and value.default not in value.choices %}
+                    {% if value.default is defined and (not value.choices or (value.default not in value.choices)) %}
                         <b>Default:</b><br/><div style="color: blue">@{ value.default | tojson | escape }@</div>
                     {% endif %}
                 </td>

With your change the default value won't be printed unless there are choices.

[phospi]

Ahh... good to know. You are right. Your suggestion fixes the issue.

[phospi]

Solved merge conflict. This PR is ready for merge.

[phospi]

Can someone please review this PR?
@goneri @Qalthos @NilashishC ?

[phospi]

Can someone please review this PR this year?
@goneri @Qalthos @NilashishC @ashwini-mhatre?