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.

Sign up for GitHub

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

add test to avoid nbsphinx issue with template extensions #518

Closed
wants to merge 5 commits into from
Closed

add test to avoid nbsphinx issue with template extensions #518

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
66cc47d
add test to avoid nbsphinx issue with template extensions
mpacer Jan 22, 2017
9a30050
clean up logic and execution
mpacer Jan 22, 2017
5120882
Include self, reposition private logging function
mpacer Jan 22, 2017
7fc2ef4
Improve test coverage
mpacer Jan 23, 2017
c780c8b
Remove pass
mpacer Jan 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions nbconvert/exporters/templateexporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,13 @@ def __init__(self, loader, extension):


def get_source(self, environment, template):

try:
return self.loader.get_source(environment, template)
except TemplateNotFound:
if template.endswith(self.extension):
raise TemplateNotFound(template)
return self.loader.get_source(environment, template+self.extension)
return self.loader.get_source(environment, template + self.extension)

def list_templates(self):
return self.loader.list_templates()
Expand Down Expand Up @@ -203,6 +204,12 @@ def __init__(self, config=None, **kw):
self.observe(self._invalidate_template_cache,
list(self.traits(affects_template=True)))

def _log_template_loading(self,template_file):
""" abstract away some of the loggging for finding templates
"""
self.log.debug("Attempting to load template %s", template_file)
self.log.debug(" template_path: %s", os.pathsep.join(self.template_path))
pass
Copy link
Member

Choose a reason for hiding this comment

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

extra pass


def _load_template(self):
"""Load the Jinja template object from the template file
Expand All @@ -217,9 +224,26 @@ def _load_template(self):
# template by name with extension added, then try loading the template
# as if the name is explicitly specified.
template_file = self.template_file
self.log.debug("Attempting to load template %s", template_file)
self.log.debug(" template_path: %s", os.pathsep.join(self.template_path))
return self.environment.get_template(template_file)

try:
self._log_template_loading(template_file)
return self.environment.get_template(template_file)
except TemplateNotFound:
if self.template_file.endswith(self.template_extension):
try:
template_file = self.template_file[:-len(self.template_extension)]
self._log_template_loading(template_file)
return self.environment.get_template(template_file)
except TemplateNotFound:
raise TemplateNotFound(template_file)
else:
try:
template_file = self.template_file + self.template_extension
self._log_template_loading(template_file)
return self.environment.get_template(template_file)
except TemplateNotFound:
raise TemplateNotFound(template_file)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@mpacer mpacer Jan 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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"!).

Copy link
Member Author

@mpacer mpacer Jan 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@takluyver takluyver Jan 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.



def from_notebook_node(self, nb, resources=None, **kw):
"""
Expand Down
21 changes: 18 additions & 3 deletions nbconvert/exporters/tests/test_templateexporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,25 @@ def test_in_memory_template(self):
# creates a class that uses this template with the template_file argument
# converts an empty notebook using this mechanism
my_loader = DictLoader({'my_template': "{%- extends 'rst.tpl' -%}"})

class MyExporter(TemplateExporter):
template_file = 'my_template'

exporter = MyExporter(extra_loaders=[my_loader])
nb = v4.new_notebook()
out, resources = exporter.from_notebook_node(nb)

def test_in_memory_template_extensions(self):
# Loads in an in memory template using jinja2.DictLoader
# creates a class that uses this template with the template_file argument
# converts an empty notebook using this mechanism
my_loader = DictLoader({'my_template.tpl': "{%- extends 'rst.tpl' -%}"})

class MyExporter(TemplateExporter):
template_file = 'my_template'

exporter = MyExporter(extra_loaders=[my_loader])
nb = v4.new_notebook()
out, resources = exporter.from_notebook_node(nb)

def test_fail_to_find_template_file(self):
# Create exporter with invalid template file, check that it doesn't
Expand All @@ -144,8 +155,12 @@ def test_fail_to_find_template_file(self):
with pytest.raises(TemplateNotFound):
out, resources = exporter.from_notebook_node(nb)



template_2 = 'does_not_exist'
exporter = TemplateExporter(template_file=template_2)
assert template_2 not in exporter.environment.list_templates(extensions=['tpl'])
nb = v4.new_notebook()
with pytest.raises(TemplateNotFound):
out, resources = exporter.from_notebook_node(nb)

def _make_exporter(self, config=None):
# Create the exporter instance, make sure to set a template name since
Expand Down