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 request argument to TemplateResponse #2191

Merged
merged 11 commits into from
Jul 13, 2023

Conversation

alex-oleshkevich
Copy link
Member

@alex-oleshkevich alex-oleshkevich commented Jun 22, 2023

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.

@alex-oleshkevich alex-oleshkevich added the refactor Refactor code label Jun 22, 2023
@alex-oleshkevich alex-oleshkevich changed the title Make request a required argument of TemplateResponse Add request argument to TemplateResponse Jun 22, 2023
Copy link
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.

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
Copy link
Member Author

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
Copy link
Member

Kludex commented Jun 22, 2023

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
Copy link
Member Author

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
Copy link
Member

Kludex commented Jun 23, 2023

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:

  1. 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.
  2. 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`.
  3. 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
Copy link
Member Author

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

@Kludex
Copy link
Member

Kludex commented Jun 30, 2023

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
Copy link
Member Author

Copy link
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.

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Also, the @deprecated PEP was still not accepted.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, so we have a comment here.

Copy link
Member

Choose a reason for hiding this comment

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

yeah yeah, just explaining for future reference 👀

@@ -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')
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

starlette/templating.py Outdated Show resolved Hide resolved
starlette/templating.py Outdated Show resolved Hide resolved
starlette/templating.py Outdated Show resolved Hide resolved
Comment on lines +209 to +210
if "request" not in kwargs.get("context", {}):
raise ValueError('context must include a "request" key')
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This preserves current behavior.

@Kludex
Copy link
Member

Kludex commented Jul 13, 2023

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.

ptrstn added a commit to ptrstn/starlette-admin that referenced this pull request Aug 31, 2024
Addresses jowilf#574

DeprecationWarning: The `name` is not the first parameter anymore. The first parameter should be the `Request` instance.
  Replace `TemplateResponse(name, {"request": request})` by `TemplateResponse(request, name)`.
    warnings.warn(

Before Starlette 0.29.0, the name was the first parameter.

Also, before that, in previous versions, the request object was passed as part of the key-value pairs in the context for Jinja2.

See:
- https://www.starlette.io/release-notes/#0290
- encode/starlette#2191
- https://github.com/encode/starlette/blob/c78c9aac17a4d68e0647252310044502f1b7da71/starlette/templating.py#L166-L178
- https://fastapi.tiangolo.com/reference/templating/#fastapi.templating.Jinja2Templates.TemplateResponse
- https://fastapi.tiangolo.com/advanced/templates/#using-jinja2templates
jowilf pushed a commit to jowilf/starlette-admin that referenced this pull request Aug 31, 2024
* Ignore .venv

* Fix DeprecationWarning: The `name` is not the first parameter anymore.

Addresses #574

DeprecationWarning: The `name` is not the first parameter anymore. The first parameter should be the `Request` instance.
  Replace `TemplateResponse(name, {"request": request})` by `TemplateResponse(request, name)`.
    warnings.warn(

Before Starlette 0.29.0, the name was the first parameter.

Also, before that, in previous versions, the request object was passed as part of the key-value pairs in the context for Jinja2.

See:
- https://www.starlette.io/release-notes/#0290
- encode/starlette#2191
- https://github.com/encode/starlette/blob/c78c9aac17a4d68e0647252310044502f1b7da71/starlette/templating.py#L166-L178
- https://fastapi.tiangolo.com/reference/templating/#fastapi.templating.Jinja2Templates.TemplateResponse
- https://fastapi.tiangolo.com/advanced/templates/#using-jinja2templates

* Fix DeprecationWarning: Extra environment options are deprecated.

Resolves #574

According to the Starlette 0.28.0 release notes, the **env_options parameter in Jinja2Templates has been deprecated in favor of the new env parameter. The relevant pull request #2159 explains this change.

See:
- encode/starlette#2159
- https://www.starlette.io/release-notes/#0280

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants