Make Jinja2Templates.get_env private & rename#1218
Make Jinja2Templates.get_env private & rename#1218JayH5 merged 2 commits intoKludex:masterfrom aminalaee:make-jinja2-get-env-internal
Conversation
starlette/templating.py
Outdated
| self.env = self._get_env(directory) | ||
|
|
||
| def get_env(self, directory: str) -> "jinja2.Environment": | ||
| def _get_env(self, directory: str) -> "jinja2.Environment": |
There was a problem hiding this comment.
@JayH5 I've been thinking about this kind of things:
| def _get_env(self, directory: str) -> "jinja2.Environment": | |
| def get_env(self, directory: str) -> "jinja2.Environment": | |
| warnings.warn( | |
| "'get_env' is deprecated. Use '_get_env' instead.", | |
| DeprecationWarning, | |
| ) | |
| return self._get_env(directory) | |
| def _get_env(self, directory: str) -> "jinja2.Environment": |
We did it for GraphQL, but we are in a pre-stable version, so in theory we can be aggressive and remove without adding warnings 🤔
Otherwise, we should be consistent.
There was a problem hiding this comment.
I think the whole idea is to shy away from using get_env direcrly. We can change the warning to encourage using templates.env instead of templates.get_env()
There was a problem hiding this comment.
We definitely wouldn't want to suggest using a private method. We could say [...] Use the env attribute instead but even that is not equivalent to get_env.
If we wanted to be really careful we could rename this method to create_env and then call it from get_env with a warning like "this is deprecated, you probably want the env attribute, but if not use create_env". But, IMO, people should basically never want what get_env currently does and it's only confusing. I agree it should be removed.
There was a problem hiding this comment.
We definitely wouldn't want to suggest using a private method.
Yeah, I agree! I've copied the warning from another snippet. 😛
But my intention here was to bring the discussion if we should keep adding warnings in pre-1.0.
|
Thanks for the PR. I think it might make sense to rename this method |
|
Renamed to |
Closes #1194.