-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add option to preserve comments when parsing templates #2037
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
base: main
Are you sure you want to change the base?
Conversation
| elif token == TOKEN_LINECOMMENT_BEGIN: | ||
| token = TOKEN_COMMENT_BEGIN | ||
| elif token == TOKEN_LINECOMMENT_END: | ||
| token = TOKEN_COMMENT_END |
There was a problem hiding this comment.
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.
|
Not sure to understand the readthedocs build failure. |
|
|
||
|
|
||
| class Comment(Stmt): | ||
| """A template comment.""" |
There was a problem hiding this comment.
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?
|
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. |
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. |
|
This would break jinja2schema: https://github.com/aromanovich/jinja2schema. It hasn't been updated in 7 years though. |
|
Other than that, almost all uses of |
| if not isinstance(body_node, (nodes.Output, nodes.Comment)) or ( | ||
| isinstance(body_node, nodes.Output) |
There was a problem hiding this comment.
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
|
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 🙂 |
Add thepreserve_commentsparameter toEnvironment.parseto preserve comments in template ASTs.Create a new
Commentnode, and always preserve those in ASTs obtained fromEnvironment.parse.Fixes #1967.
should fail without the change.
.. versionchanged::entries in any relevant code docs.