-
-
Notifications
You must be signed in to change notification settings - Fork 951
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
Make Jinja2Templates.get_env private & rename #1218
Conversation
starlette/templating.py
Outdated
|
||
def get_env(self, directory: str) -> "jinja2.Environment": | ||
def _get_env(self, directory: str) -> "jinja2.Environment": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Closes #1194.