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 env argument to Jinja2Templates, remove **env_options. (ref #2134) #2159

Merged
merged 17 commits into from Jun 5, 2023

Conversation

alex-oleshkevich
Copy link
Member

This PR adds env argument to Jinja2Templates class and removes **env_options.

Motivation
See this discussion - #2134

  1. arguments of Jinja2Templates configure only this class and nothing else
  2. users can configure jinja2.Environment and use it in the app without limits
  3. the Environment class may be used by other app components, not just by Starlette

This is a breaking change!

Example

import jinja2
from starlette.templating import Jinja2Templates

env = jinja2.Environment(...)
templates = Jinja2Templates(env=env)

@NickColley
Copy link

I like this, it meets the "starlette has a simple opinionated way" need whilst also allowing the user to do something really complicated without the starlette project having to think about a new API at all - since it falls to a shared idea of the Jinja environment which is referenced and used in many different jinja projects.

@Kludex
Copy link
Sponsor Member

Kludex commented May 31, 2023

Can we deprecate the **env_options first, then remove it on v1?

@@ -64,18 +65,27 @@ class Jinja2Templates:

def __init__(
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can we overload this?

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea

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.

With the overload and the rebase, we are done here 👍

Comment on lines 86 to 92
self,
directory: typing.Union[str, PathLike],
**env_options: typing.Any,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Suggested change
self,
directory: typing.Union[str, PathLike],
**env_options: typing.Any,
self, directory: typing.Union[str, PathLike], **env_options: typing.Any

tests/test_templates.py Show resolved Hide resolved
@Kludex
Copy link
Sponsor Member

Kludex commented Jun 3, 2023

I've rebased it.

@Kludex Kludex mentioned this pull request Jun 4, 2023
@alex-oleshkevich
Copy link
Member Author

I had to make all other arguments as kw-only to make overloading work.

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.

Thanks 👍

Let's accept the context_processors being a kw-only as a breaking change.

docs/templates.md Outdated Show resolved Hide resolved
@@ -61,7 +61,7 @@ templates.env.filters['marked'] = marked_filter

## Using custom jinja2.Environment instance

Starlette also accepts a preconfigured `jinja2.Environment` instance.
Starlette also accepts a preconfigured [`jinja2.Environment`](https://jinja.palletsprojects.com/en/3.0.x/api/#api) instance.
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think this doesn't render very well on mkdocs

Copy link
Member Author

Choose a reason for hiding this comment

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

it looks good
image

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Ah ok. Great 👍

@Kludex
Copy link
Sponsor Member

Kludex commented Jun 5, 2023

Thanks 👍

@Kludex Kludex merged commit d155d3b into encode:master Jun 5, 2023
5 checks passed
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

3 participants