cylc lint - incorrect warnings about jinja2 and comments

[ColemanTom]

Description

The linter incorrectly warns about jinja2 comments, for example if a # is in that line even if it isn't a comment.

Reproducible Example

Add to an environment section:

A = $((10#$VAR + {{ SOMETHING }}))
B = "$((10#$VAR + {{ SOMETHING }}))"

Both of the above will output a warning, so it is not a quoted vs unquoted issue.

[S011] flow.cylc:25: Cylc will process commented Jinja2!
[S011] flow.cylc:26: Cylc will process commented Jinja2!

Expected Behaviour

No warning should be issued because this is valid syntax for shell and the # is not a comment for Cylc.

[hjoliver]

Yeah I guess we need more sophisticated parsing to identify # comments.

Cylc will process commented Jinja2!

Also, this message should be a little more explicit, to make it clear we're not talking about Jinja2 comments {#

[oliver-sanders]

There's only so far we can go with this simple regex linter, suggest demoting this to "INFO" level.

[ColemanTom]

Yes, a regex linter can't handle all situations very easily. The regex would get ever more complex as new edge cases come up. Without knowing the internals, I know Cylc does something to remove comments in its processing, so could that be leveraged. Instead of removing comments, it extracts and returns them (with a line number reference perhaps), and those are then processed?

[oliver-sanders]

For the linter we need to stick to a simple regex approach as:

• We need to be able to lint the templating language too.
• We need to be able to lint branches controlled by the templating language even if they are not active under the current configuration (e.g. if a template variable controls a Jinja2 if switch, we should lint both branches of the if in all cases).
• It is not necessarily possible to run Jinja2 logic for a workflow definition anyway, many require template variables to be defined or optional configs (a Rose thing) to be activated.
• Parsing the file would be too costly to fit with our plans for the command.

We can potentially allow the linter to be a little bit stateful, but we can't run the file through the parsec parser.

[hjoliver]

How about we close this issue by simply extending the regex a little to assume that Cylc comments either start at ^ (line beginning) or are preceded by a space #.

Most of the time that will be true, and it will avoid flagging shell substitution syntax. A few missed positives is better than false positives, and likely inconsequential anyhow.

[ColemanTom]

You could have two versions. One which assumes as you stated, and is a warning. Another which is stricter but is info, which is off by default, and covers all? #5542 would perhaps help with this topic too, although I do dislike cluttering files with lint directives.

[ColemanTom]

Also, I don't know all edge cases, but in this one, ignoring #$ would protect against this case I think.

[ColemanTom]

@hjoliver if you want I think you can close this as #5841 resolved the issue reported at least. Up to you though in case you wanted it kept for the other discussion taking place.

[oliver-sanders]

Closed by #5841