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 option to preserve comments when parsing templates #2037

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pawamoy
Copy link
Contributor

@pawamoy pawamoy commented Oct 10, 2024

Add the preserve_comments parameter to Environment.parse to preserve comments in template ASTs.

Create a new Comment node, and always preserve those in ASTs obtained from Environment.parse.

Fixes #1967.

  • Add tests that demonstrate the correct behavior of the change. Tests
    should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.

src/jinja2/environment.py Outdated Show resolved Hide resolved
src/jinja2/lexer.py Outdated Show resolved Hide resolved
src/jinja2/lexer.py Outdated Show resolved Hide resolved
src/jinja2/lexer.py Outdated Show resolved Hide resolved
Comment on lines +654 to +657
elif token == TOKEN_LINECOMMENT_BEGIN:
token = TOKEN_COMMENT_BEGIN
elif token == TOKEN_LINECOMMENT_END:
token = TOKEN_COMMENT_END
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: Possible (small?) performance hit. Not sure how to avoid it.

src/jinja2/parser.py Outdated Show resolved Hide resolved
src/jinja2/parser.py Outdated Show resolved Hide resolved
src/jinja2/parser.py Show resolved Hide resolved
@pawamoy pawamoy marked this pull request as ready for review October 10, 2024 16:27
@pawamoy
Copy link
Contributor Author

pawamoy commented Oct 10, 2024

Not sure to understand the readthedocs build failure.

@@ -715,6 +715,13 @@ def as_const(self, eval_ctx: t.Optional[EvalContext] = None) -> t.Any:
return self.expr2.as_const(eval_ctx)


class Comment(Stmt):
"""A template comment."""
Copy link
Contributor Author

@pawamoy pawamoy Oct 10, 2024

Choose a reason for hiding this comment

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

suggestion: Could maybe add .. versionadded:: 3.2, is that a valid directive?

@davidism
Copy link
Member

davidism commented Oct 10, 2024

The docs failure is unrelated and something I'm actively working on, you can ignore it.

What if we just added these nodes to the AST unconditionally? I'm not sure that would be a breaking change, as an existing walker would skip those nodes. I'd rather not have more configuration if possible.

@pawamoy
Copy link
Contributor Author

pawamoy commented Oct 10, 2024

What if we just added these nodes to the AST unconditionally? I'm not sure that would be a breaking change, as an existing walker would skip those nodes. I'd rather not have more configuration if possible.

Totally fine by me! I definitely understand not wanting more configuration. I'll run a few code searches on GitHub to see if this would break anything, and then do the change.

@pawamoy
Copy link
Contributor Author

pawamoy commented Oct 10, 2024

This would break jinja2schema: https://github.com/aromanovich/jinja2schema.

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/media/data/dev/jinja/.venv/lib/python3.12/site-packages/jinja2schema/core.py", line 49, in infer_from_ast
    rv = visit(ast, {}, config)
         ^^^^^^^^^^^^^^^^^^^^^^
  File "/media/data/dev/jinja/.venv/lib/python3.12/site-packages/jinja2schema/visitors/util.py", line 20, in visit
    structure = visit_many(node.body, macroses, config)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/media/data/dev/jinja/.venv/lib/python3.12/site-packages/jinja2schema/visitors/util.py", line 37, in visit_many
    structure = visit(node, macroses, config, predicted_struct_cls, return_struct_cls)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/media/data/dev/jinja/.venv/lib/python3.12/site-packages/jinja2schema/visitors/util.py", line 14, in visit
    structure = visit_stmt(node, macroses, config)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/media/data/dev/jinja/.venv/lib/python3.12/site-packages/jinja2schema/visitors/stmt.py", line 53, in visit_stmt
    raise Exception('stmt visitor for {0} is not found'.format(type(ast)))
Exception: stmt visitor for <class 'jinja2.nodes.Comment'> is not found

It hasn't been updated in 7 years though.

@pawamoy
Copy link
Contributor Author

pawamoy commented Oct 10, 2024

Other than that, almost all uses of env.parse I find on GitHub are just to pass the AST to meta.find_undeclared_variables.

Comment on lines +318 to +319
if not isinstance(body_node, (nodes.Output, nodes.Comment)) or (
isinstance(body_node, nodes.Output)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: This was needed to fix the logic and two failing tests:

FAILED tests/test_inheritance.py::TestInheritance::test_level1_required - jinja2.exceptions.TemplateSyntaxError: Required blocks can only contain comments or whitespace
FAILED tests/test_inheritance.py::TestInheritance::test_invalid_required - jinja2.exceptions.TemplateSyntaxError: Required blocks can only contain comments or whitespace

@pawamoy
Copy link
Contributor Author

pawamoy commented Oct 10, 2024

I've pushed a fixup commit that removes the new parameter and changes the code so that comments are always preserved in ASTs. Makes the overall change much smaller 🙂

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

Successfully merging this pull request may close these issues.

Option to preserve comments in the AST
2 participants