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

FileSystemLoader include paths in error #1663

Merged
merged 2 commits into from
Dec 20, 2024
Merged

Conversation

Yourun-proger
Copy link
Contributor

@Yourun-proger Yourun-proger commented May 2, 2022

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@ThiefMaster
Copy link
Member

I don't find this particularly useful to be honest. Especially in a larger Flask app with many blueprints (which IIRC. all contribute to the search path) this would make the error message extremely bloated with information that's not particularly useful in most cases...

@@ -215,7 +215,9 @@ def uptodate() -> bool:

# Use normpath to convert Windows altsep to sep.
return contents, os.path.normpath(filename), uptodate
raise TemplateNotFound(template)
raise TemplateNotFound(
f"{template} not found in the following search path(s): {self.searchpath}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, maybe add a starred expression here

@Yourun-proger
Copy link
Contributor Author

Probably. I saw now that in airflow has already come up with a patch. Maybe the truth is that everyone should change on their own, not at the library level.
In short, I can close this.

@Yourun-proger
Copy link
Contributor Author

Well, a parameter like ignore or verbose is definitely stupid....

@uranusjr
Copy link

uranusjr commented May 2, 2022

A middle ground for this would be to include searchpath on the exception object as an attribute, but not shown in the error message. This should be good enough for most usages; they can simply do

try:
    template = get_template(filename)
except TemplateNotFound as e:
    raise MyCustomException(f"{filename} not found in {e.searchpath}")

instead of having to dig through the template loaders to find the context.

@Yourun-proger
Copy link
Contributor Author

@uranusjr, thanks for the reasonable offer. I think that it can satisfy everyone. If no one minds, I'll take care of it.

@Yourun-proger
Copy link
Contributor Author

It's me again. I made a patch that looks like this:

diff --git a/src/jinja2/exceptions.py b/src/jinja2/exceptions.py
index 082ebe8..6031bee 100644
--- a/src/jinja2/exceptions.py
+++ b/src/jinja2/exceptions.py
@@ -1,4 +1,5 @@
 import typing as t
+import os

 if t.TYPE_CHECKING:
     from .runtime import Undefined
@@ -18,6 +19,9 @@ class TemplateError(Exception):
 class TemplateNotFound(IOError, LookupError, TemplateError):
     """Raised if a template does not exist.

+    .. versionchanged:: 3.1.3
+        Added new parameter "search path"
+
     .. versionchanged:: 2.11
         If the given name is :class:`Undefined` and no message was
         provided, an :exc:`UndefinedError` is raised.
@@ -31,6 +35,7 @@ class TemplateNotFound(IOError, LookupError, TemplateError):
         self,
         name: t.Optional[t.Union[str, "Undefined"]],
         message: t.Optional[str] = None,
+        searchpath: t.Optional[t.Union[str, os.PathLike, t.Sequence[t.Union[str, os.PathLike]]]] = None,
     ) -> None:
         IOError.__init__(self, name)

@@ -45,6 +50,8 @@ class TemplateNotFound(IOError, LookupError, TemplateError):
         self.message = message
         self.name = name
         self.templates = [name]
+        # Transforms of searchpath are not required here.
+        self.searchpath = searchpath

     def __str__(self) -> str:
         return str(self.message)
@@ -55,6 +62,9 @@ class TemplatesNotFound(TemplateNotFound):
     are selected.  This is a subclass of :class:`TemplateNotFound`
     exception, so just catching the base exception will catch both.

+    .. versionchanged:: 3.1.3
+        Added new parameter "search path"
+
     .. versionchanged:: 2.11
         If a name in the list of names is :class:`Undefined`, a message
         about it being undefined is shown rather than the empty string.
@@ -66,6 +76,7 @@ class TemplatesNotFound(TemplateNotFound):
         self,
         names: t.Sequence[t.Union[str, "Undefined"]] = (),
         message: t.Optional[str] = None,
+        searchpath: t.Optional[t.Union[str, os.PathLike, t.Sequence[t.Union[str, os.PathLike]]]] = None,
     ) -> None:
         if message is None:
             from .runtime import Undefined

and

diff --git a/src/jinja2/loaders.py b/src/jinja2/loaders.py
index d2c39c1..e398c2d 100644
--- a/src/jinja2/loaders.py
+++ b/src/jinja2/loaders.py
@@ -215,9 +215,7 @@ class FileSystemLoader(BaseLoader):

             # Use normpath to convert Windows altsep to sep.
             return contents, os.path.normpath(filename), uptodate
-        raise TemplateNotFound(
-            f"{template} not found in the following search path(s): {self.searchpath}"
-        )
+        raise TemplateNotFound(template, searchpath=self.searchpath)

     def list_templates(self) -> t.List[str]:
         found = set()

In principle, now you can achieve the desired behavior and do something like:

try:
    template = my_file_system_loader.get_source(some_env, filename)
except TemplateNotFound as e:
    raise MyCustomException(f"{filename} not found in {e.searchpath}")

As you may have understood, I now need to add the keyword argument searchpath wherever TemplateNotFound and TemplatesNotFound are raised, which is feasible in principle, but seems like some kind of bad decision? What do you think about this? Can you advise me something?

@davidism davidism added this to the 3.1.5 milestone Dec 20, 2024
@davidism
Copy link
Member

Flask won't be affected by this, as it raises a simple TemplateNotFound(name) rather than any particular error message from the combined loaders. I don't want to add special attributes to the errors, as searchpath would only be relevant to FileSystemLoader. There's another PR for improving the PackageLoader error message as well, and TemplateNotFound already includes a way to specify the template name and a detailed message, so this seems fine.

@davidism davidism changed the title New verbose error message FileSystemLoader include paths in error Dec 20, 2024
@davidism davidism merged commit 58a358f into pallets:stable Dec 20, 2024
12 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants