Lint: Narrow down scope of Jinja2 in comment warning

[wxtim]

Description

Jinja2 flow controls ({% statement %}) are not safe in Cylc comments - they will still be evaluated. Jinja2 variable subs ({{myvar}}) are fine and should be passed by Cylc Lint.

At the moment Cylc Lint objects in both cases.

Reproducible Example

#!jinja2
{% set x = 'Hello' %} 
# Legit use of Jinja2 to customize a comment:
# I want to say {{x}}

> cylc lint workflow
[S011] flow.cylc:6: Cylc will process commented Jinja2!

[hjoliver]

Jinja2 variable subs ({{myvar}}) are fine and should be passed by Cylc Lint.

Does not validate (string contains a newline that breaks the Cylc comment):

#!Jinja2
{% set x = """
Hello""" %}
# I want to say {{x}}
...

Jinja2 flow controls ({% statement %}) are not safe in Cylc comments - they will still be evaluated.

Validates OK:

#!Jinja2
{% set x = ["hello", "bye"] %}
# I want to say{% for y in x %} {{y}}{% endfor %}
...

So: I think it is OK to warn about any Jinja2 inside a Cylc comment. It may or may not be valid, but either way users need to be aware of the danger. And that it is of limited use at best, as the processed comments can only be seen with cylc view -j.

[hjoliver]

The warning could be worded better though, e.g.:

> cylc lint workflow
[S011] flow.cylc:6: check this Jinja2 generates a valid Cylc comment (e.g. beware of newlines).

[wxtim]

The warning could be worded better though, e.g.:

> cylc lint workflow
[S011] flow.cylc:6: check this Jinja2 generates a valid Cylc comment (e.g. beware of newlines).

I don't think that this reflects by biggest concern - users thinking that the following example has removed the control statement and that some setting will be set to 42:

# {% if True == False %}
  some setting =42
# {% endif %}

[MetRonnie]

Jinja2 variable subs ({{myvar}}) are fine and should be passed by Cylc Lint.

Arguably not fine, a user may comment out such a line and not set myvar, then be surprised by Jinja2 failing on the fact that myvar has not been set

[hjoliver]

Given that Jinja2 templates care nothing about the embedding text, technically it is equally legit to put Jinja2 code inside or outside Cylc comments. The only thing that matters is that valid Cylc config is the end result. So it's a bit dangerous to make any assumptions about what the user wants here.

I often {{print}} into Cylc comments to check the result of some Jinja code via cylc view -j. The view command doesn't require the flow.cylc to validate, but sometimes it's convenient to leave the print in there for future use of view, even if it just generates a Cylc comment that then gets ignored.

So I think we should only do this if we extend cylc lint to allow "suggestions" that don't come with non-zero (fail) exit status.

I don't think that this reflects by biggest concern - users thinking that the following example has removed the control statement and that some setting will be set to 42:

# {% if True == False %}
 some setting =42
# {% endif %}

Definitely worth a warning for this, but "failing" it could be problematic for the reasons I've just given.

... athough, linting is not pass/fail validation, and # noqa is available for use. Where should we draw the line, for this sort of thing?