Skip to content
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

Add support for auto-indented blocks #919

Closed
wants to merge 2 commits into from

Conversation

tomas-mazak
Copy link

@tomas-mazak tomas-mazak commented Oct 31, 2018

Fixes #178

Blocks now support a new syntax {%* ... %} that aligns the indentation of
multiline string with the block statement itself. This is especially
useful when templating YAML or other languages where indentation matters. Example:

labels.j2:

tla: webtool
env: {{ env }}

deployment.yaml.j2:

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    {% include 'labels.j2' %}
  name: webtool
spec:
  selector:
    matchLabels:
      {% include 'labels.j2' %}
  strategy:
    type: Recreate

...renders to broken YAML:

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    tla: webtool
env: qa
  name: webtool
spec:
  selector:
    matchLabels:
      tla: webtool
env: qa
  strategy:
    type: Recreate

deployment_new_syntax.yaml.j2:

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    {%* include 'labels.j2' %}
  name: webtool
spec:
  selector:
    matchLabels:
      {%* include 'labels.j2' %}
  strategy:
    type: Recreate

...renders correctly:

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    tla: webtool
    env: qa
  name: webtool
spec:
  selector:
    matchLabels:
      tla: webtool
      env: qa
  strategy:
    type: Recreate

@tomas-mazak
Copy link
Author

This actually only supports blocks now, but if the idea is generally accepted, I am happy to extend support to variables as well

@tomas-mazak
Copy link
Author

I've updated the PR with variable support, so now {{* ... }} syntax is supported as well

@Ashish-Bansal
Copy link

@davidism Any update on this ? I would love to have this feature merged.

@davidism
Copy link
Member

davidism commented Nov 20, 2018

There's now no way to trim leading newlines, unless I'm missing that you can combine * and -.

@tomas-mazak
Copy link
Author

@davidism yeah, I only implemented the feature I immediately needed for now... I am happy to look into extensions if required to get this merged

@tomas-mazak
Copy link
Author

@davidism Please let me know if you require any amendments to this PR, I am happy to do some extra work on it, but would really like to see this merged.

@stan1y
Copy link

stan1y commented Dec 19, 2018

Does it support extends syntax as well?

@tomas-mazak
Copy link
Author

@stan1y I haven't tested it with extend, but it should work with any kind of block (as it wraps the whole block in a indent filter under the hood)

@caidanw
Copy link

caidanw commented Jan 3, 2019

I would love to have this feature as well, looks great!

@pallets pallets deleted a comment from tomas-mazak Jan 31, 2019
@stan1y
Copy link

stan1y commented Jan 31, 2019

I can confirm that it worked with extends as expected. Very useful.

@thirtytwobits
Copy link

I pulled this into a fork I'm using to generate source code. It's an essential feature in some regards unless someone else knows a better way to do this?

@153957
Copy link

153957 commented Jun 28, 2019

👍 I like this new functionality!
However, this PR is missing new tests..

@tomas-mazak tomas-mazak reopened this Jun 28, 2019
Fixes pallets#178

Blocks now support a new syntax '{%* ... %}' that alings the indentation of
multiline string with the block statement itself. This is especially
useful with YAML or other languages where indentation matter. Example:

labels.j2:
```
tla: webtool
env: {{ env }}
```

deployment.yaml.j2:
```
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    {% include 'labels.j2' %}
  name: webtool
spec:
  selector:
    matchLabels:
      {% include 'labels.j2' %}
  strategy:
    type: Recreate
```
...renders to broken YAML:
```
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    tla: webtool
env: qa
  name: webtool
spec:
  selector:
    matchLabels:
      tla: webtool
env: qa
  strategy:
    type: Recreate
```

deployment_new_syntax.yaml.j2:
```
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    {%* include 'labels.j2' %}
  name: webtool
spec:
  selector:
    matchLabels:
      {%* include 'labels.j2' %}
  strategy:
    type: Recreate
```
...renders correctly:
```
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    tla: webtool
    env: qa
  name: webtool
spec:
  selector:
    matchLabels:
      tla: webtool
      env: qa
  strategy:
    type: Recreate
```
Now `{{* ... }}` syntax can be used too, for auto-indentation of
multiline expressions. Useful for macros for example:

Template:
```
{%- macro labels() -%}
tla: webtool
env: {{ env }}
{% endmacro -%}
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    {{* labels() }}
  name: webtool
spec:
  selector:
    matchLabels:
      {{* labels() }}
  strategy:
    type: Recreate
```

Renders to:
```
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    tla: webtool
    env: qa
  name: webtool
spec:
  selector:
    matchLabels:
      tla: webtool
      env: qa
  strategy:
    type: Recreate
```
@tomas-mazak
Copy link
Author

@153957 Care to provide more details? I've closed/reopened the PR and the tests are failing now with Python nightly but that doesn't seem to be related to this PR's code ...

@153957
Copy link

153957 commented Jun 28, 2019

This PR adds new functionality but there are no changes to any files in /tests/, should new functionality not be tested?

/edit
Something similar to this existing test.

def test_raw3(self, env):
# The second newline after baz exists because it is AFTER the {% raw %} and is ignored.
env = Environment(lstrip_blocks=True, trim_blocks=True)
tmpl = env.from_string("bar\n{% raw %}\n {{baz}}2 spaces\n{% endraw %}\nfoo")
assert tmpl.render(baz='test') == "bar\n\n {{baz}}2 spaces\nfoo"

@tomas-mazak
Copy link
Author

@153957 ah, thanks for heads up Arne, I will look into that

@KlavsKlavsen
Copy link

This would be a really really nice feature to have.. We are currently using jinja to template yaml files.. and have hit this exact issue with no solution.. (it works in helm - which have a jinja-like templating format)..

@tomas-mazak tomas-mazak reopened this Aug 22, 2019
@davidism
Copy link
Member

davidism commented Oct 7, 2019

Due to #858, which fixed a bug that caused catastrophic backtracking while lexing whitespace, this no longer merges, and it's not trivial for me to adapt this to the new code.

Comments:

  • Implementing this by automatically producing a filter call during parsing seems incorrect. Why can't the whitespace be preserved during lexing, as other whitespace handling is done?

  • Needs tests. It's possible to apply this to a lot of tokens ({% raw %}, {%, {{, line statements) but it's not clear if all the results will make sense. Fix lexer being extremely slow on large amounts of whitespace. Fixes #857 #858 had to modify the TOKEN_RAW_BEGIN rule as well, is that applicable here?

  • It would be nice to make this work with whitespace control.

  • Maybe it would make more sense to implement this as another environment option similar to trim_blocks and lstrip_blocks. Maybe block_indent? That way you're opting in to the indent behavior everywhere, and you don't need to add new syntax.

@davidism

This comment has been minimized.

@OJFord

This comment has been minimized.

@davidism

This comment has been minimized.

@davidism
Copy link
Member

davidism commented Oct 8, 2019

Another thing to consider here is that we also need to consider dedent as well as indent. It's common to indent both within the markup you're generating as well as the Jinja blocks, resulting in extra long indentation once the Jinja blocks are gone.

def validate(data):
    {% for name in required %}
        if "{{ name }}" not in data:
            raise ValidationError("{{ name }} is required")
    {% endfor %}

    return True

This is a contrived example, but the use of indentation to visually distinguish blocks becomes more useful as the template becomes more complex. So what you really want is to ensure that the first non-whitespace character in the contents starts at the same position as the start tag, and then apply that to the rest of the contents.

@davidism
Copy link
Member

Thanks for working on this! I'm closing it now based on my comments above. Need to find a better solution for this.

@davidism davidism closed this Oct 13, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preserving whitespace prefix in multiline strings
10 participants