T4353: alternative Jinja2 style guide

[dmbaturin]

Change Summary

Points

• Indentation should be outside of template tags, not inside them.
• Content inside template blocks should be indented.
• Unrelated blocks must be separated by newlines.

Details

First of all, I'm sorry for not stepping in earlier when Christian first introduced the j2lint linter! If the alternative style guide is accepted, I'm ready to reformat templates that he already updated.

The main change I want to propose is indenting template tags rather than statements inside template tags. That is, it should look like this:

{% for foo in bar %}
    {% if foo is defined %}
    {% endif %}
{% endfor %}

rather than like this:

{% for foo in bar %}
{%     if foo is defined %}
{%     endif %}
{% endfor %}

The main reason is that when opening tags are indented, any editor can reindent those templates. With indentation inside tags, a template writer will need a special editor setup, if tools to make such a setup in any editor even exist now.

I also, somewhat subjectively, believe that "outside" indents also make code much easier to read, since {% at the line start draws attention to itself. Besides, the opening tag is a part of the statement.

Also, the style that j2linter enforces appears to be unique to Arista. Official Jinja2 docs use "outside" indentation.

j2lint considerations

j2lint, unfortunately, cannot be configured to use outside indentation. It's, sadly, a very opinionated tool, and its opinions are clearly against those of Jinja2 authors. ;)

I also looked inside the code and it's not possible to easily modify to support both outside and inside indentation.

It also disallows whitespace control tags by default, even though they are normal and useful.

However! That code isn't sophisticated either. It only recognized basic Jinja2 keywords (if/endif, for/endfor) etc. and doesn't try to do abstract interpretation for correctness checks, so a similar or better linter can be recreated in a few days of undisturbed effort.

For now we may disable its indentation and whitespace control checks, but keep the rest to check basic template syntax correctness automatically at least.

Handling existing templates

Since Christian indented existing templates consistently, we can convert the "inside" style to "outside" with a script. Here's a prototype:

#!/usr/bin/env python3

import re
import sys

with open(sys.argv[1], 'r') as f:
    for l in f.readlines():
        res = re.search(r'^\{%(\s+)\S', l)
        if res:
            # Extract the original indent
            spaces = res.group(1)

            # Replace the indent within the tag with a single space
            l = re.sub(r'(\{%)(\s+)(\S)', '\\1 \\3', l)

            # Since indents inside tags include an extra space,
            # we need to subtract it
            sys.stdout.write(spaces[1:])

        sys.stdout.write(l)

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [x] Code style update (formatting, renaming)
• [ ] Refactoring (no functional changes)
• [ ] Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
• [ ] Other (please describe):

Related Task(s)

• https://phabricator.vyos.net/T4353

Proposed changes

Checklist:

• [x] I have read the CONTRIBUTING document
• [x] I have linked this PR to one or more Phabricator Task(s)
• [ ] I have run the components SMOKETESTS if applicable
• [x] My commit headlines contain a valid Task id
• [x] My change requires a change to the documentation
• [ ] I have updated the documentation accordingly

[sever-sever]

@dmbaturin In general, I like the idea of this format:

 {% if foo %}
     {% if bar %}
         baz
     {% endif %}
 {% endif %}

But some configurations may look ugly. Wouldn't this prevent some daemons from reading the configuration?

[jestabro]

Discussed in today's dev meeting: we need to evaluate whether any daemons are leading-white-space sensitive, and how easily leading white-space can be controlled during rendering.

[github-actions[bot]]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[dmbaturin]

We may re-evaluate in the future.