Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where Air's Jinja rendering system was stringifying all variables in the template context, preventing normal Jinja operations on non-tag objects like lists and dictionaries. The fix constrains stringification to only objects that inherit from BaseTag, allowing Jinja to process other data types normally.
- Modified context processing to only stringify
BaseTaginstances - Added test coverage for Jinja's handling of lists and dictionaries
- Created supporting test template to verify proper rendering behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/air/templating.py |
Updated context processing logic to conditionally stringify only BaseTag instances |
tests/test_templating.py |
Added test to verify Jinja can process lists and dicts without stringification |
tests/templates/lists_and_dicts.html |
Created test template that renders metadata and iterates over list items |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
378f383 to
df35751
Compare
|
LGTM One other option that could make this a bit more readable (subjectively, to my eyes) would be: def _jinja_context_item(self, item):
"""Prepare an item for processing by Jinja
BaseTag instances are converted to string.
All other objects are handled by Jinja directly.
"""
if isinstance(item, BaseTag):
return str(item)
return item
...
context = {k: self._jinja_context_item(v) for k, v in context.items()}It's a matter of taste and I wouldn't block on merging - the change looks perfect - but this more verbose version is self-documenting and easier to extend if the need ever arises. |
| @@ -184,7 +185,7 @@ def _prepare_context(self, context: dict[Any, Any] | None, kwargs: dict[Any, Any | |||
|
|
|||
There was a problem hiding this comment.
Type of the __call__ function should be updated accordingly to _TemplateResponse or str | _TemplateResponse
There was a problem hiding this comment.
This suggested change throws all kinds of type errors. For now I'm merging this in, will look at the typing issue in a separate PR.
@bluerosej discovered that Air's Jinja rendering system is stringifying everything. She also provided a fix in pyladiesmanila/VoicesOfPyLadiesManila#18. We expand on that excellent fix by constraining it to just stringify BaseTag-inherited items so that Jinja can operate normally. Co-Authored-By: Rose J <195869163+bluerosej@users.noreply.github.com>
Introduced _jinja_context_item helper to consistently convert BaseTag instances to strings before rendering with Jinja. Updated JinjaRenderer and Renderer to use this helper for context preparation. Also added .cov_html and .benchmarks to skip list in pyproject.toml. Co-Authored-By: Eleanor Berger <46264+intellectronica@users.noreply.github.com> Co-Authored-By: Md Al Amin <alaminopu.me@gmail.com>
c33d9c1 to
e18d419
Compare
|
@intellectronica I made the change you suggested and gave you attribution. Thanks! |
Issue(s)
#547
Description
@bluerosej discovered that Air's Jinja rendering system is stringifying everything. She also provided a fix in pyladiesmanila/VoicesOfPyLadiesManila#18. We expand on that excellent fix by constraining it to just stringify BaseTag-inherited items so that Jinja can operate normally.
Pull request type
Please check the type of change your PR introduces:
Pull request tasks
The following have been completed for this task:
examplesfolderChecklist
just testandjust qa, ensuring my code changes passes all existing tests