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

Add AsyncTemplateResponse to Jinja2Templates #1701

Closed
wants to merge 8 commits into from

Conversation

amisadmin
Copy link

Help Jinja2 make it easier and faster to use async template rendering.

@amisadmin amisadmin force-pushed the master branch 2 times, most recently from e164aad to 19b660c Compare June 23, 2022 12:30
@Kludex Kludex requested a review from aminalaee June 28, 2022 18:33
Copy link
Member

@aminalaee aminalaee left a comment

Choose a reason for hiding this comment

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

I think this looks good, I'd wait for @tomchristie 's input on this too.

tests/test_templates.py Show resolved Hide resolved
starlette/templating.py Outdated Show resolved Hide resolved
starlette/templating.py Show resolved Hide resolved
@euri10
Copy link
Member

euri10 commented Jun 29, 2022

@aminalaee
Copy link
Member

https://github.com/encode/starlette/blob/master/docs/templates.md#asynchronous-template-rendering

Yeah I mentioned if this is going to be merged the docs should be updated.


templates = Jinja2Templates(directory='templates', enable_async=True)

async def get_name():
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest some kind of comment noting that this'd actually only make sense in the case of an actual async operation. (Eg. an async database lookup.)

Copy link
Author

Choose a reason for hiding this comment

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

Ok, thanks for your advice.

@tomchristie
Copy link
Member

is this not valid anymore https://github.com/encode/starlette/blob/master/docs/templates.md#asynchronous-template-rendering ?

Subjective. Personally I think it's probably decent advice. But. 🤷‍♂️

@Kludex Kludex self-requested a review June 30, 2022 19:15
@Kludex Kludex added this to the Version 0.21.0 milestone Jun 30, 2022
starlette/templating.py Outdated Show resolved Hide resolved
starlette/templating.py Outdated Show resolved Hide resolved
routes=[Route("/", endpoint=homepage)],
)
templates = Jinja2Templates(directory=str(tmpdir), enable_async=True)
templates.env.globals["get_name"] = get_name
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Why is this not added on line 46?

Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

We also need to tweak a bit the documentation, as the introduced section doesn't connect well with the previous section. 🤔

@Kludex Kludex added feature New feature or request staticfiles Static file serving labels Jul 22, 2022
@Kludex
Copy link
Sponsor Member

Kludex commented Aug 12, 2022

@amisadmin Are you still interested on working on this PR?

@amisadmin
Copy link
Author

@amisadmin Are you still interested on working on this PR?

Yes, I am. But I'm a terrible document writer.

amisadmin and others added 3 commits August 13, 2022 10:38
Co-authored-by: Marcelo Trylesinski <[email protected]>
Co-authored-by: Marcelo Trylesinski <[email protected]>
@Kludex Kludex mentioned this pull request Aug 18, 2022
8 tasks
@Kludex Kludex modified the milestones: Version 0.21.0, Version 0.22.0 Sep 21, 2022
@Kludex Kludex modified the milestones: Version 0.22.0, Version 0.23.0 Nov 20, 2022
@Kludex Kludex mentioned this pull request Dec 12, 2022
7 tasks
@Kludex
Copy link
Sponsor Member

Kludex commented Feb 4, 2023

I'm going to pass on this for the time being. We can come back to this after 1.0 is released.

Thanks for the PR, and sorry for the waiting time.

@Kludex Kludex closed this Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request staticfiles Static file serving
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants