Add `request` argument to `TemplateResponse` #2191

alex-oleshkevich · 2023-06-22T15:11:42Z

This PR makes request a required argument to TemplateResponse.
Also, as the request is now passed explicitly, the template context can be optional.

Why?
The request is mandatory in the template response. However, the class requires the request to be passed via context and there are asserts to check it. I found it better to pass the request explicitly. It is similar to what Django does.

Was

TemplateResponse('index.html', {'request': request})

Become

TemplateResponse(request, 'index.html')

This is a breaking change.
Ref #2174

EDIT [@Kludex]: This is not a breaking change anymore - It now introduces deprecation warnings instead.

Kludex

This is a breaking change.

Please make both request and context optional, and raise a deprecation warning when the "request" is in the context.

alex-oleshkevich · 2023-06-22T15:32:19Z

This is a breaking change.

Please make both request and context optional, and raise a deprecation warning when the "request" is in the context.

Actually, the request must be the first argument.
Moving it after name also is not okay because name and context both are in the same domain and good to have them together:

TemplateResponse(request, 'index.html', {})
TemplateResponse('index.html', request, {})

Kludex · 2023-06-22T15:41:32Z

Actually, the request must be the first argument.

"Must" is a bit strong, isn't it? 👀

I don't think the argument here justifies the breaking change. It's just cosmetics.

alex-oleshkevich · 2023-06-22T16:22:52Z

Actually, the request must be the first argument.

"Must" is a bit strong, isn't it? eyes

I don't think the argument here justifies the breaking change. It's just cosmetics.

Can you suggest an alternative? Seems that I am stuck..

Kludex · 2023-06-23T16:27:33Z

Hmm... I was writing a huge text as to why my last suggestion was the only one... And I actually got an idea... 🤔

You can make the two first arguments as str | Request | None = None, and context as Dict | None = None. Meaning:

Check if the first parameter:
1.1. isinstance(request, str) -> Use it as name; raise a DeprecationWarning - say it's not the 1st param anymore.
1.2. isinstance(request, Request) -> Work as expected.
Check if the second parameter:
1.1. isinstance(name, str) -> Work as expected.
1.2. isinstance(name, dict) -> Use it as context; raise DeprecationWarning- say it's not the 2nd param anymore. 1.2.1. Check ifrequestis incontext, as it's happening right now, but if it's, raise DeprecationWarning`.
Check if third parameter ???

My guess is this is going to look hideous, but it's not a breaking change, and we can easily remove all this logic on 1.0.

alex-oleshkevich · 2023-06-28T18:46:38Z

Okay, it worked. The code is a bit ugly but it's okay for the transition period.

Kludex · 2023-06-30T07:36:12Z

The code is a bit ugly

Eh... Yeah... It was within my predictions hahaha

Would you like to open a PR against this already to show how it would look like on 1.0? I think it will make it easier to get this in.

alex-oleshkevich · 2023-06-30T09:56:35Z

@Kludex https://github.com/encode/starlette/pull/2199/files

Kludex

I've added some comments. @alex-oleshkevich Can you check them, please?

Kludex · 2023-07-06T17:17:04Z

starlette/templating.py

        status_code: int = 200,
        headers: typing.Optional[typing.Mapping[str, str]] = None,
        media_type: typing.Optional[str] = None,
        background: typing.Optional[BackgroundTask] = None,
    ) -> _TemplateResponse:
-        if "request" not in context:
-            raise ValueError('context must include a "request" key')
+        # Deprecated usage


Unfortunately, we can't use @typing_extensions.deprecated without making typing_extensions mandatory for Starlette.

Also, the @deprecated PEP was still not accepted.

yes, so we have a comment here.

yeah yeah, just explaining for future reference 👀

Kludex · 2023-07-06T17:18:30Z

docs/templates.md

@@ -22,7 +22,7 @@ from starlette.staticfiles import StaticFiles
 templates = Jinja2Templates(directory='templates')

 async def homepage(request):
-    return templates.TemplateResponse('index.html', {'request': request})
+    return templates.TemplateResponse(request, 'index.html')


Is this the only thing in the documentation that uses TemplateResponse?

starlette/templating.py

Kludex · 2023-07-06T18:01:29Z

starlette/templating.py

+                if "request" not in kwargs.get("context", {}):
+                    raise ValueError('context must include a "request" key')


This error is being raised with the warning above, can we make sure we only have the warning if this doesn't raise?

This preserves current behavior.

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

…ette into templates-request

Kludex · 2023-07-13T07:57:42Z

Thanks @alex-oleshkevich 🙏

For the record, this was accepted given that it is not a breaking change, and that we already have a follow-up PR to clean-up everything for V1: #2199. 🙏

The idea is to warn the users now, and then remove the DeprecationWarning for V1.

make request a required argument of TemplateResponse

2af4d82

alex-oleshkevich added the refactor Refactor code label Jun 22, 2023

alex-oleshkevich changed the title ~~Make request a required argument of TemplateResponse~~ Add request argument to TemplateResponse Jun 22, 2023

Kludex requested changes Jun 22, 2023

View reviewed changes

add compatibility between old and new argument set of TemplateResponse

6847bc5

alex-oleshkevich added 2 commits June 28, 2023 20:51

rename test

dda3bc0

replace AsyncMock with MagicMock

527f4c3

alex-oleshkevich mentioned this pull request Jul 3, 2023

Add request argument to TemplateResponse (v1.x) #2199

Closed

Kludex mentioned this pull request Jul 6, 2023

Version 0.29.0 #2209

Merged

3 tasks

Kludex requested changes Jul 6, 2023

View reviewed changes

Kludex and others added 7 commits July 6, 2023 20:03

Merge branch 'master' into templates-request

1dc2743

Update starlette/templating.py

c110b23

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

Update starlette/templating.py

0e9f58d

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>

simplify code

1a2a387

Merge branch 'templates-request' of github.com:alex-oleshkevich/starl…

ab6a689

…ette into templates-request

fix tests

b3418eb

Merge branch 'master' into templates-request

f67e075

Kludex approved these changes Jul 13, 2023

View reviewed changes

Kludex merged commit 0308681 into encode:master Jul 13, 2023
5 checks passed

falkoschindler mentioned this pull request Dec 27, 2023

Error mesage: 'int' object has no attribute 'items' when run example code zauberzeug/nicegui#2247

Closed

kitrio mentioned this pull request Jan 15, 2024

starlette 버전업에 따른 템플릿 함수파라미터 변경사항 gnuboard/g6#249

Closed

apiwat-chantawibul mentioned this pull request Mar 1, 2024

Bug: _TemplateResponse is still relying on request key from context which maybe different from request argument to TemplateResponse. #2531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `request` argument to `TemplateResponse` #2191

Add `request` argument to `TemplateResponse` #2191

alex-oleshkevich commented Jun 22, 2023 •

edited by Kludex

Kludex left a comment

alex-oleshkevich commented Jun 22, 2023

Kludex commented Jun 22, 2023

alex-oleshkevich commented Jun 22, 2023

Kludex commented Jun 23, 2023

alex-oleshkevich commented Jun 28, 2023

Kludex commented Jun 30, 2023

alex-oleshkevich commented Jun 30, 2023

Kludex left a comment

Kludex Jul 6, 2023

alex-oleshkevich Jul 10, 2023

Kludex Jul 13, 2023

Kludex Jul 6, 2023

alex-oleshkevich Jul 10, 2023

Kludex Jul 6, 2023

alex-oleshkevich Jul 10, 2023

Kludex commented Jul 13, 2023

		if "request" not in kwargs.get("context", {}):
		raise ValueError('context must include a "request" key')

Add request argument to TemplateResponse #2191

Add request argument to TemplateResponse #2191

Conversation

alex-oleshkevich commented Jun 22, 2023 • edited by Kludex

Kludex left a comment

Choose a reason for hiding this comment

alex-oleshkevich commented Jun 22, 2023

Kludex commented Jun 22, 2023

alex-oleshkevich commented Jun 22, 2023

Kludex commented Jun 23, 2023

alex-oleshkevich commented Jun 28, 2023

Kludex commented Jun 30, 2023

alex-oleshkevich commented Jun 30, 2023

Kludex left a comment

Choose a reason for hiding this comment

Kludex Jul 6, 2023

Choose a reason for hiding this comment

alex-oleshkevich Jul 10, 2023

Choose a reason for hiding this comment

Kludex Jul 13, 2023

Choose a reason for hiding this comment

Kludex Jul 6, 2023

Choose a reason for hiding this comment

alex-oleshkevich Jul 10, 2023

Choose a reason for hiding this comment

Kludex Jul 6, 2023

Choose a reason for hiding this comment

alex-oleshkevich Jul 10, 2023

Choose a reason for hiding this comment

Kludex commented Jul 13, 2023

Add `request` argument to `TemplateResponse` #2191

Add `request` argument to `TemplateResponse` #2191

alex-oleshkevich commented Jun 22, 2023 •

edited by Kludex