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

feat: add minjinja #2250

Merged
merged 25 commits into from
Sep 8, 2023
Merged

feat: add minjinja #2250

merged 25 commits into from
Sep 8, 2023

Conversation

euri10
Copy link
Contributor

@euri10 euri10 commented Aug 28, 2023

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR

Description

  • add minijinja to the template implenetation
  • (we conuld make create a contrib/templates folder to host jinja, mako and this...)
  • test fails, not sure if we should adapt them given this: url_for support in templates? mitsuhiko/minijinja#314 or ask upstream, I'm not sure I understand fully the reason,

Close Issue(s)

@Goldziher
Copy link
Contributor

So, why would anyone want to use this instead of jinja?

@euri10
Copy link
Contributor Author

euri10 commented Aug 28, 2023

from https://github.com/mitsuhiko/minijinja/tree/main/minijinja-py

You might want to use MiniJinja instead of Jinja2 when the full feature set of Jinja2 is not required and you want to have the same rendering experience of a data set between Rust and Python.

some people may be interested to test the speed also, I dont know.

but in any case it should be seen as a jinja replacement, the python bindings page explains quite well the main differences.

@euri10 euri10 marked this pull request as ready for review August 31, 2023 04:44
@euri10 euri10 requested review from a team as code owners August 31, 2023 04:44
@euri10
Copy link
Contributor Author

euri10 commented Sep 2, 2023

I honestly dont understand the error in the CI here

pyproject.toml Outdated Show resolved Hide resolved
@Goldziher
Copy link
Contributor

Please update the documentation and readme

Copy link
Contributor

@Goldziher Goldziher left a comment

Choose a reason for hiding this comment

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

PR looks good, but needs update to docs and readme before proceeding.

fix: add tests for docs/examples/templating
fix: seems mako prints a \n at end of templates if the html conatins one
fix: typos in tabs names
@euri10
Copy link
Contributor Author

euri10 commented Sep 8, 2023

ok I have a small annoyance here in new tests:

  • in returning_templates_mako.py the test fails with
'Hello <strong>Mako</strong>\n' != 'Hello <strong>Mako</strong>'

Expected :'Hello <strong>Mako</strong>'
Actual   :'Hello <strong>Mako</strong>\n'

it's because the file hello.html.mako does contain a \n at the end (in fact linters pyright forces you to have it)

I'm caught in a chiken and egg issue, if I remove the \n from the template file hello.html.mako then pyright will fail in the CI but new tests pass,

should the test for mako be changed to reflect that slight discrepency between jinja and mako ?

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/2250

@Goldziher Goldziher merged commit bb08a53 into litestar-org:main Sep 8, 2023
17 checks passed
@euri10 euri10 deleted the euri10/minjinja branch January 10, 2024 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants