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

Reimplement jinja[spacing] to avoid use of regex #2306

Merged
merged 1 commit into from Aug 22, 2022

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Aug 18, 2022

This change rewrites jinja2 rule to make it use jinaja lexer and
avoid use of regexes for parsing, as they cause too many false
positives.

Fixes: #2301
Fixes: #2285
Fixes: #2241
Fixes: #2209
Fixes: #2208
Fixes: #2120

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fascinating use of the jinja lexer.
In the lexer-related work I've been doing, I've been using TOKEN_* constants from jinja:
https://github.com/pallets/jinja/blob/main/src/jinja2/lexer.py#L61-L109

Typically I do from jinja2 import lexer as j2tokens and then use j2tokens.TOKEN_OPERATOR or similar. What do you think of that idiom?

src/ansiblelint/rules/jinja.py Outdated Show resolved Hide resolved
src/ansiblelint/rules/jinja.py Outdated Show resolved Hide resolved
@ssbarnea
Copy link
Member Author

@drybjed I am not sure how well you will take this but this change seems to uncover at least one genuine jinja2 syntax error in debops repository, but you might find yourself bit overwhelmed by the amount of feedback linter is providing:

Take it easy! And to quote from thread,... I hope you get paid by the hour... ;)

@ssbarnea ssbarnea force-pushed the fix/jinja branch 3 times, most recently from 5900e3f to e8e1db2 Compare August 21, 2022 15:34
@ssbarnea
Copy link
Member Author

@cognifloyd There are 4 reformatting test that are failing which I do not know how to fix. If you can have a look it would be great. That PR took all my weekend and I might really benefit from another pair of eyes to deal with these last ones.

All these tests were results of different errors I found while testing formatting of jinja2 expression on all eco repositories, so we should be pleased about the results.

Few minutes ago I got another idea,... what if I would try to use black itself to format the snippets, assuming that it might be used programately. Still, doing that would not be a joy as I would have to identify blocks of tokens and recombine them before I feed them to black,.. so maybe not a very good idea.

Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ew. Yes those are difficult. Here are some ideas to hopefully give you an idea of what to look at next.

src/ansiblelint/rules/jinja.py Show resolved Hide resolved
src/ansiblelint/rules/jinja.py Show resolved Hide resolved
@drybjed
Copy link
Contributor

drybjed commented Aug 21, 2022

@ssbarnea Thanks for the heads up. I'll test the proposed changes and make fixes ASAP.

@ssbarnea ssbarnea force-pushed the fix/jinja branch 2 times, most recently from 553472f to c4a1965 Compare August 21, 2022 20:06
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: ansible#2301 ansible#2285 ansible#2241 ansible#2209 ansible#2208 ansible#2120
hswong3i added a commit to alvistack/ansible-role-postgres that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-packer that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-picard that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-postfix that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-perforce that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-sonobuoy that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-python that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-podman that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-runc that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-skopeo that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-scribus that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-sshd that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-sqlite that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-teams that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-swap that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-transmission that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-thunderbird that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-svn that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-vlc that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-vagrant that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-teamviewer that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-zram_generator that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-virtualbox that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-timezone that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-uget that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-wireguard that referenced this pull request Aug 22, 2022
hswong3i added a commit to alvistack/ansible-role-vim that referenced this pull request Aug 22, 2022
@drybjed
Copy link
Contributor

drybjed commented Aug 31, 2022

@drybjed I am not sure how well you will take this but this change seems to uncover at least one genuine jinja2 syntax error in debops repository, but you might find yourself bit overwhelmed by the amount of feedback linter is providing:

Take it easy! And to quote from thread,... I hope you get paid by the hour... ;)

@ssbarnea I'm currently working on fixing all these warnings on ansible-lint v6.5.0 so that DebOps can be used to fix false-positives in v6.5.1. I wish for one thing - separate Jinja identation into its own rule so that it could be ignored if desired. There are small snippets of Jinja where I had to drop idented if/endif statements, and that's alright with me, but for larger pieces of Jinja, for example more extensive templates, please please please let me keep identation - it makes things much more readable.

@drybjed
Copy link
Contributor

drybjed commented Aug 31, 2022

@ssbarnea All issues in DebOps have been fixed in the master branch. There are some false-positives though on the latest ansible-lint release.

@ssbarnea
Copy link
Member Author

@drybjed Thanks. I got some progress today on them but again failed to pass all tests. That jinja spacing is a bit of a can of worms.

I already introduce a condition to skip the spacing checks when newlines are found inside the jinja blocks, basically because we assume user added them for indentation purposes. The scope of checking spaces was more towards simple blocks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants