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

When using a custom Jinja loader, directory parameter is unnecessary but still required. #2134

Closed
NickColley opened this issue Apr 26, 2023 · 9 comments
Labels
clean up Refinement to existing functionality staticfiles Static file serving
Milestone

Comments

@NickColley
Copy link

In our project we're using Jinja templates like so:

from jinja2 import ChoiceLoader, PackageLoader, PrefixLoader, FileSystemLoader
from fastapi.templating import Jinja2Templates

template_loader = ChoiceLoader(
    [
        FileSystemLoader("name/web/templates"),
        PrefixLoader({"govuk_frontend_jinja": PackageLoader("govuk_frontend_jinja")}),
    ]
)

# Use a custom template loader and avoid the default directory option.
templates = Jinja2Templates(directory="null", loader=template_loader)

At the moment we use a generic "null" which is not used since in the code if you provide your own loader the default file system loader is bypassed.

loader = jinja2.FileSystemLoader(directory)
env_options.setdefault("loader", loader)

With this in mind, if a loader is passed should the directory parameter be entirely optional?

Should the API allow this?

templates = Jinja2Templates(loader=template_loader)
@euri10
Copy link
Member

euri10 commented Apr 26, 2023

see this #1214 for the background as to why the API is what is

I agree with @alex-oleshkevich that currently the special case for loader is unsatisfactory if you want fine-grained control.

@NickColley
Copy link
Author

I think this could also be confusing if people expect the directory to be merged with the loader or something like this. It's not clear what's happening at the moment. But seems like that thread has covered that confusion somewhat.

@Kludex Kludex assigned Kludex and unassigned Kludex May 24, 2023
@Kludex
Copy link
Sponsor Member

Kludex commented May 24, 2023

@alex-oleshkevich wdyt?

@Kludex Kludex added staticfiles Static file serving need confirmation This issue needs confirmation. labels May 24, 2023
@alex-oleshkevich
Copy link
Member

My opinion: we need only two mutually exclusive parameters for this purpose: env: jinja2.Environment and directory: str|list[str]. The latter one is a shortcut. This covers all cases I ever faced.

I disagree with having loader and kwargs as, IMO, it is not a nice API because:

  • not obvious
  • mixes env-specific and class-specific parameters
  • it is not clear what are kwargs about. Do they include env and class arguments or only env?

cc @Kludex

@NickColley
Copy link
Author

I agree that the environment should be exposed since you can then do whatever you'd like and starlette doesnt have to concern itself with duplicating interfaces.

I've also ran into issues where a third party Jinja project assumed I had access to the environment, which we dont with starlette.

@alex-oleshkevich
Copy link
Member

I made a PR to demonstrate the changes we discussed - #2159

@Kludex
Copy link
Sponsor Member

Kludex commented May 31, 2023

I've suggested to first deprecate it: #2159 (comment)

Then on v1 we can remove it.

@Kludex Kludex added clean up Refinement to existing functionality and removed need confirmation This issue needs confirmation. labels May 31, 2023
@Kludex Kludex added this to the Version 1.0 milestone May 31, 2023
Kludex added a commit that referenced this issue Jun 5, 2023
) (#2159)

Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@alex-oleshkevich
Copy link
Member

The solution is merged. We can close this issue.

@NickColley
Copy link
Author

Thank you for your work on this much appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up Refinement to existing functionality staticfiles Static file serving
Projects
None yet
Development

No branches or pull requests

4 participants