Skip to content

Commit

Permalink
Reimplement jinja[spacing] to avoid use of regex
Browse files Browse the repository at this point in the history
This change rewrites jinja2 rule to make it use jinaja lex parser and
avoid use of regexes for parsing, as they cause too many false
positives.

Fixes: #2301 #2285 #2241 #2209 #2208 #2120
  • Loading branch information
ssbarnea committed Aug 20, 2022
1 parent 1cefe54 commit 782ba5a
Show file tree
Hide file tree
Showing 8 changed files with 369 additions and 133 deletions.
2 changes: 2 additions & 0 deletions .config/dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,10 @@ unignored
unimported
unindented
unjinja
unlex
unnormalized
unskippable
unspaced
unsubscriptable
untemplated
uwsgi
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ jobs:
WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:TOX_PARALLEL_NO_SPINNER
# Number of expected test passes, safety measure for accidental skip of
# tests. Update value if you add/remove tests.
PYTEST_REQPASS: 661
PYTEST_REQPASS: 682

steps:
- name: Activate WSL1
Expand Down
9 changes: 5 additions & 4 deletions examples/playbooks/jinja-spacing.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
---
# Should raise jinja[spacing] at tasks line 23, 26, 29, 54, 65
- hosts: all
- name: Fixture for testing jinja2[spacing]
hosts: all
tasks:
- name: Good variable format
ansible.builtin.debug:
Expand All @@ -18,7 +19,7 @@
ansible.builtin.debug:
msg: "{{ '{{' }}"
- name: Jinja escaping allowed
# noqa: 306
# noqa: risky-shell-pipe
ansible.builtin.shell: docker info --format '{{ '{{' }}json .Swarm.LocalNodeState{{ '}}' }}' | tr -d '"'
changed_when: false
- name: Jinja whitespace control allowed
Expand Down Expand Up @@ -56,10 +57,10 @@
vars:
cases:
case1: >-
http://example.com/{{
http://foo.com/{{
case1 }}
case2: >-
http://example.com/{{
http://bar.com/{{
case2 }}
ansible.builtin.debug:
var: cases
Expand Down
18 changes: 9 additions & 9 deletions examples/playbooks/vars/jinja-spacing.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
# Should raise jinja[spacing] at line [12, 13, 14, 27, 33], at following variables.
# Should raise jinja[spacing] at line [14, 15, 16, 17, 18, 19, 22, 32, 38], at following variables.
# ".bad_var_1", ".bad_var_2", ".bad_var_3", ".invalid_multiline_nested_json", ".invalid_nested_json"
good_var_1: "{{ good_format }}"
good_var_2: "Value: {{ good_format }}"
Expand All @@ -11,12 +11,12 @@ jinja_whitespace_control: |
{{ good_format }}/
{{- good_format }}
{{- good_format -}}
bad_var_1: "{{bad_format}}"
bad_var_2: "Value: {{ bad_format}}"
bad_var_3: "{{bad_format }}"
bad_var_4: "{{ bad_format|filter }}"
bad_var_5: "Value: {{ bad_format |filter }}"
bad_var_6: "{{ bad_format| filter }}"
bad_var_1: "{{bad_format}}" # <-- 1
bad_var_2: "Value: {{ bad_format}}" # <-- 2
bad_var_3: "{{bad_format }}" # <-- 3
bad_var_4: "{{ bad_format|filter }}" # <-- 4
bad_var_5: "Value: {{ bad_format |filter }}" # <-- 5
bad_var_6: "{{ bad_format| filter }}" # <-- 6
# spell-checker: disable-next-line
non_jinja_var: "data = ${lookup{$local_part}lsearch{/etc/aliases}}" # noqa: jinja[spacing]
json_inside_jinja: "{{ {'test': {'subtest': variable}} }}"
Expand All @@ -29,13 +29,13 @@ multiline_vars: # Assert that no false positive on multiline exists
http://example.com/{{
case2 }}
valid_nested_json: "{{ {'dummy_2': {'nested_dummy_1': 'value_1', 'nested_dummy_2': value_2}} | combine(dummy_1) }}"
invalid_nested_json: "{{ {'dummy_2': {'nested_dummy_1': 'value_1', 'nested_dummy_2': value_2}} | combine(dummy_1)}}"
invalid_nested_json: "{{ {'dummy_2': {'nested_dummy_1': 'value_1', 'nested_dummy_2': value_2}} | combine(dummy_1)}}" # <-- 7

valid_multiline_nested_json: >-
{{ {'dummy_2': {'nested_dummy_1': value_1,
'nested_dummy_2': value_2}} |
combine(dummy_1) }}
invalid_multiline_nested_json: >-
invalid_multiline_nested_json: >- # <-- 8
{{ {'dummy_2': {'nested_dummy_1': value_1,
'nested_dummy_2': value_2}} |
combine(dummy_1)}}
2 changes: 1 addition & 1 deletion src/ansiblelint/_internal/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def getmatches(self, file: Lintable) -> List[MatchError]:
try:
matches.extend(method(file))
except Exception as exc: # pylint: disable=broad-except
_logger.debug(
_logger.warning(
"Ignored exception from %s.%s: %s",
self.__class__.__name__,
method,
Expand Down
13 changes: 9 additions & 4 deletions src/ansiblelint/rules/jinja.md
Original file line number Diff line number Diff line change
@@ -1,20 +1,25 @@
## jinja

This rule can report problems related to jinja2 string templates. The current
version can report `jinja[spacing]` when there are no spaces between variables
and operators, including filters, like `{{ var_name | filter }}`. This
improves readability and makes it less likely to introduce typos.
version can report:

- `jinja[spacing]` when there are no spaces between variables
and operators, including filters, like `{{ var_name | filter }}`. This
improves readability and makes it less likely to introduce typos.
- `jinja[invalid]` when the jinja2 template is invalid

### Problematic code

```yaml
---
foo: "{{some|dict2items}}"
foo: "{{some|dict2items}}" # <-- jinja[spacing]
bar: "{{ & }}" # <-- jinja[invalid]
```

### Correct code

```yaml
---
foo: "{{ some | dict2items }}"
bar: "{{ '&' }}"
```

0 comments on commit 782ba5a

Please sign in to comment.