Fix and improve template rendering subsystem #181
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the template rendering subsystem to better distinguish between synchronous and asynchronous rendering modes. It introduces separate environment instances for sync and async operations, renames the main async render function from render_async to render, and adds new synchronous render functions render_sync and render_sync_native.
Key changes include:
- Separation of async and sync template environments with dedicated singleton instances
- API simplification:
render_async()→render()with newrender_sync()functions - Addition of
syncparameter to environment and template creation functions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| qtoggleserver/utils/template.py | Refactored to support separate sync/async environments, renamed internal variables, updated render functions to new naming convention |
| tests/unit/qtoggleserver/utils/test_template.py | Added comprehensive tests for sync/async environment creation, updated tests for new render function names |
| tests/integration/utils/test_template.py | Renamed test functions to match new API (test_render_async → test_render, etc.), added tests for new sync render functions |
| tests/unit/qtoggleserver/core/expressions/test_expression.py | Renamed helper class from TestableExpression to _TestableExpression following Python convention for private test helpers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_async(self, mocker): | ||
| """Should obtain the environment using `get_env(False)` and call `from_string()` on it with the source, | ||
| returning the resulted template.""" | ||
|
|
||
| spy_make.assert_called_once_with("{{some_source}}") | ||
| mock_tmpl.render.assert_called_once_with({"some": "context"}) | ||
| assert result is mock_tmpl.render() | ||
| mock_env = mocker.Mock() | ||
| spy_get_env = mocker.patch("qtoggleserver.utils.template.get_env", return_value=mock_env) | ||
|
|
||
| tmpl = make("{{some_source}}") | ||
|
|
||
| spy_get_env.assert_called_once_with(False) | ||
| mock_env.from_string.assert_called_once_with("{{some_source}}") | ||
| assert tmpl is mock_env.from_string() |
There was a problem hiding this comment.
This test method test_async is a duplicate of test_default (lines 187-198) with identical implementation and expectations. Since make() defaults to async mode (sync=False), both tests verify the same behavior. Consider removing this duplicate test or modifying it to test a distinct scenario.
| def test_async(self, mocker): | ||
| """Should obtain the environment using `get_native_env(False)` and call `from_string()` on it with the source, | ||
| returning the resulted template.""" | ||
|
|
||
| mock_env = mocker.Mock() | ||
| spy_get_native_env = mocker.patch("qtoggleserver.utils.template.get_native_env", return_value=mock_env) | ||
|
|
||
| tmpl = make_native("{{some_source}}") | ||
|
|
||
| spy_get_native_env.assert_called_once_with(False) | ||
| mock_env.from_string.assert_called_once_with("{{some_source}}") | ||
| assert tmpl is mock_env.from_string() |
There was a problem hiding this comment.
This test method test_async is a duplicate of test_default (lines 228-239) with identical implementation and expectations. Since make_native() defaults to async mode (sync=False), both tests verify the same behavior. Consider removing this duplicate test or modifying it to test a distinct scenario.
No description provided.