Repository: encode/starlette
Pull request: Make Jinja2Templates.get_env private & rename
Closes #1194.

Problem statement:
Consider making `starlette.templating.Jinja2Templates.get_env` "protected"
Hi,
Thank you very much for your project! We are using it intensively through FastAPI.

While I was exploring the code base and looking how to use `Jinja2Templates`, I noticed the method `get_env`. At first I thought it was a getter for the environment, but eventually realized it is only called in the constructor of the class.

It would make much more sense to me if the method was called `_get_env`. This would signal that it should not be used outside of the class (or at least that the caller needs to know what she is doing).

For example, if I want to add filters or globals to the environment, I should write:

```
templates = Jinja2Templates(...)
templates.env.globals[...] = ...
```

instead of:

```
templates = Jinja2Templates(...)
templates.get_env().globals[...] = ...
```

As of current implementation, the second snippet has no effect.

Thank you very much for considering this request!

Hints from issue discussion:

Commit message: make-jinja2-get-env-internal

Patch to review:
diff --git a/starlette/templating.py b/starlette/templating.py
index 64fdbd14f..d80f31673 100644
--- a/starlette/templating.py
+++ b/starlette/templating.py
@@ -56,9 +56,9 @@ class Jinja2Templates:
 
     def __init__(self, directory: str) -> None:
         assert jinja2 is not None, "jinja2 must be installed to use Jinja2Templates"
-        self.env = self.get_env(directory)
+        self.env = self._get_env(directory)
 
-    def get_env(self, directory: str) -> "jinja2.Environment":
+    def _get_env(self, directory: str) -> "jinja2.Environment":
         @pass_context
         def url_for(context: dict, name: str, **path_params: typing.Any) -> str:
             request = context["request"]


Code review comments:
diff_hunk: @@ -56,9 +56,9 @@ class Jinja2Templates:
 
     def __init__(self, directory: str) -> None:
         assert jinja2 is not None, "jinja2 must be installed to use Jinja2Templates"
-        self.env = self.get_env(directory)
+        self.env = self._get_env(directory)
 
-    def get_env(self, directory: str) -> "jinja2.Environment":
+    def _get_env(self, directory: str) -> "jinja2.Environment":
path: starlette/templating.py
text: @user2:
@user1 I've been thinking about this kind of things:

```suggestion
    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 :thinking: 

Otherwise, we should be consistent.

@author:
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()`

@user1:
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.

@user2:
> 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.