add test to avoid nbsphinx issue with template extensions #518
Conversation
mpacer
force-pushed
the
template_extension_fix
branch
from
7987dae to
10ee15d
Compare
mpacer
force-pushed
the
template_extension_fix
branch
from
10ee15d to
b19a140
Compare
mpacer
force-pushed
the
template_extension_fix
branch
from
b19a140 to
66cc47d
Compare
mpacer
force-pushed
the
template_extension_fix
branch
from
e65acdc to
7fc2ef4
Compare
|
@michaelpacer This looks good to me. Ran fine locally for me too. |
| """ | ||
| self.log.debug("Attempting to load template %s", template_file) | ||
| self.log.debug(" template_path: %s", os.pathsep.join(self.template_path)) | ||
| pass |
|
+1 |
| self._log_template_loading(template_file) | ||
| return self.environment.get_template(template_file) | ||
| except TemplateNotFound: | ||
| raise TemplateNotFound(template_file) |
There was a problem hiding this comment.
Having a bunch of checks and fallbacks here is precisely what I was trying to avoid by making the ExtensionTolerantLoader, so I'd rather not put them all back in here.
There was a problem hiding this comment.
@takluyver It wasn't totally clear to me what extension tolerant meant. I would recommend adding a section to the class docstring to clarify that these checks are not needed when using ExtensionTolerantLoader. Perhaps a quick rationale sentence as the 2nd paragraph in the docstring before constructor paragraph.
There was a problem hiding this comment.
One of the primary use cases that this broke was nbsphinx. That's fixed now, but it's not obvious to me what is different between
from jinja2 import DictLoader
my_loader = DictLoader({'my_template': "{%- extends 'rst.tpl' -%}"})
class MyExporter(nbconvert.RSTExporter):
template_file = 'my_template'
exporter = MyExporter(extra_loaders=[my_loader])and
loader = jinja2.DictLoader({'nbsphinx-rst.tpl': RST_TEMPLATE})
super(Exporter, self).__init__(
template_file='nbsphinx-rst', extra_loaders=[loader],
config= traitlets.config.Config(
{'HighlightMagicsPreprocessor': {'enabled': True}}),
filters={
'markdown2rst': markdown2rst,
'get_empty_lines': _get_empty_lines,
'extract_toctree': _extract_toctree,
'get_output_type': _get_output_type,
'json_dumps': json.dumps,
})
And extension tolerant loader seems to not be activated in that case; my first attempt at this was to add additional logic to ExtensionTolerantLoader to look for the case where the extension was present and could instead be removed, but despite the logic being correct (as evidence by logging) trying to loading from there didn't work and that failure seemed to be related to the _load_template function.
There was a problem hiding this comment.
I suppose the lack of clarity of what I attempted is in part due to my trying to clean up my commit history and overwriting the earlier stuff I did adding additional logic to ExtensionTolerantLoader. I'll try do that less going forward.
There was a problem hiding this comment.
I don't think there's any significant difference between those, which don't use an extension either to declare or select the template. I think that both would be broken in 5.0, and fixed by #491.
The case that broke in nbsphinx was with nbsphinx-rst.tpl as the name where the template was defined.
There was a problem hiding this comment.
I rarely go digging into the commit history of a PR to work out what the author was thinking. There are just too many PRs for that kind of detailed inspection ;-). IMHO, the important thing is something I know you're good at: writing. When you open an issue or PR, write a clear description of the problem at hand. If a PR relates to an issue, link it ("fixes issue #517" instead of "fixes the issue"!).
There was a problem hiding this comment.
The second custom loader was broken with #491, the former was not. That's the problem this PR is fixing.
Edit: I had mistakenly quoted my own PRs version rather than the erroring code.
There was a problem hiding this comment.
Yep, the version that was broken by #491 was like this:
loader = jinja2.DictLoader({'nbsphinx-rst.tpl': RST_TEMPLATE})
super(Exporter, self).__init__(
template_file='nbsphinx-rst', extra_loaders=[loader],
...Here, the template is defined with the .tpl extension, and selected without it. That's different from the first example, where it is both defined and selected without the extension.
|
I don't think we should change this functionality - I've explained why in #517 . I'm happy to clarify the docstring of ExtensionTolerantLoader, though. |
|
Arguing over this is not a productive use of our time when the original source of the problem is fixed. In #517 it was said that there aren't too many people that are using the style of a template declaration that used in nbconvert but doesn't after the release of 5.1.1. Hopefully that is correct, because if they did use it, 5.1.1 will break their code. |
Fixes issue #517 but does so in a way I don't like, I'm going to simplify the logic next.
This does mean you can't have both a template without and with a file extension and expect straightforward importing behaviour (but on disk templates should have extensions anyway).