refactor: use jinja for comments instead of inline string formatting #84
Conversation
|
/lgtm review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🦉 lgtm Review
Score: Needs Work 🔧
🔍 Summary
This is a solid refactoring that effectively separates presentation from logic by introducing Jinja2 templates. This greatly improves the maintainability of the formatting code.
The review has identified a critical correctness issue that will cause a runtime error, along with some smaller bugs in the new Jinja templates. There are also several suggestions for quality improvements and a potential issue in the tests that should be addressed. Please review the detailed comments below.
More information
- Id:
2f414df47e6c4f61b8e71d2c1bdc156d - Model:
gemini-2.5-pro - Created at:
2025-08-22T16:54:47.977838+00:00
Usage summary
Call 1
- Request count:
1 - Request tokens:
27661 - Response tokens:
1506 - Total tokens:
35709
Call 2
- Request count:
1 - Request tokens:
21389 - Response tokens:
1444 - Total tokens:
25363
Total tokens: 61072
See the 📚 lgtm-ai repository for more information about lgtm.
scastlara
force-pushed
the
jinja
branch
2 times, most recently
from
aecf8ac to
ad22bab
Compare
|
/lgtm review |
scastlara
changed the title
There was a problem hiding this comment.
🦉 lgtm Review
Score: Nitpicks 🤓
🔍 Summary
This is a strong refactoring that improves maintainability by using Jinja2 for templating instead of f-strings. The separation of concerns between logic and presentation is a great step forward.
Before this can be merged, a few key points need to be addressed. There's a security concern regarding the disabling of auto-escaping in Jinja2, and a type hint was loosened, which reduces type safety. I've also left a couple of minor suggestions on the new Jinja templates to improve their robustness.
Overall, great work. Addressing these points will make the PR ready for merging.
More information
- Id:
432080c4148a4402bfc3c08d0b69540b - Model:
gemini-2.5-pro - Created at:
2025-08-22T17:05:38.912404+00:00
Usage summary
Call 1
- Request count:
1 - Request tokens:
27715 - Response tokens:
1222 - Total tokens:
38594
Call 2
- Request count:
1 - Request tokens:
21175 - Response tokens:
1225 - Total tokens:
30355
Total tokens: 68949
See the 📚 lgtm-ai repository for more information about lgtm.
|
/lgtm review |
There was a problem hiding this comment.
🦉 lgtm Review
Score: Nitpicks 🤓
🔍 Summary
This is a great refactoring. Moving the string formatting logic into Jinja templates is a significant improvement for readability and maintainability. The code is much cleaner and easier to manage now.
I have a few suggestions to improve it further:
- I've left a comment regarding Jinja's
autoescapesetting. Usingautoescape=Falseis a security risk, and it would be better to enable it and mark trusted variables with the| safefilter where necessary. - One of the new templates has some duplicated code that can be easily removed.
- The tests have become a bit harder to read due to the new formatting assertions. I've suggested an alternative approach that should improve their clarity.
These are relatively minor points, and overall this is a solid contribution. Great job!
More information
- Id:
7f58530a17ba4894ac65f60d74c1486b - Model:
gemini-2.5-pro - Created at:
2025-08-25T07:29:08.945788+00:00
Usage summary
Call 1
- Request count:
1 - Request tokens:
26653 - Response tokens:
1087 - Total tokens:
34469
Call 2
- Request count:
1 - Request tokens:
20294 - Response tokens:
1145 - Total tokens:
22625
Total tokens: 57094
See the 📚 lgtm-ai repository for more information about lgtm.
scastlara
force-pushed
the
jinja
branch
2 times, most recently
from
634bdc0 to
8a7101f
Compare
It was always hard working with this, and testing it was also a bit painful.
Let's use jinja to do all string formatting.
Note: jinja was a transitive dependency, so we aren't actually adding any other dep.
closes #85