-
Notifications
You must be signed in to change notification settings - Fork 27
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
Improved check for choices and default value #57
base: master
Are you sure you want to change the base?
Conversation
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. |
Ahh... good to know. You are right. Your suggestion fixes the issue. |
Solved merge conflict. This PR is ready for merge. |
Can someone please review this PR? |
Can someone please review this PR this year? |
Hi,
I created this PR because I am observing that the following
options
-documenation is parsed with an error:The error message looks like this:
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?
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 nochoices
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.