nbconvert: Jinjaless exporter base #4175

Merged
merged 15 commits into from Sep 18, 2013

Conversation

Projects
None yet
3 participants
Owner

jdfreder commented Sep 5, 2013

@Carreau said he would be away for a week and gave me permission to rebase his PR #3747 .

@minrk minrk and 1 other commented on an outdated diff Sep 5, 2013

IPython/nbconvert/exporters/export.py
@@ -100,14 +100,14 @@ def export(exporter, nb, **kw):
#Check arguments
if exporter is None:
raise TypeError("Exporter is None")
- elif not isinstance(exporter, Exporter) and not issubclass(exporter, Exporter):
+ elif not isinstance(exporter, TemplateExporter) and not issubclass(exporter, TemplateExporter):
@minrk

minrk Sep 5, 2013

Owner

shouldn't this still be Exporter?

@jdfreder

jdfreder Sep 5, 2013

Owner

Yes that would be better, good catch

@minrk minrk commented on an outdated diff Sep 5, 2013

IPython/nbconvert/exporters/export.py
raise TypeError("exporter does not inherit from Exporter (base)")
if nb is None:
raise TypeError("nb is None")
#Create the exporter
resources = kw.pop('resources', None)
- if isinstance(exporter, Exporter):
+ if isinstance(exporter, TemplateExporter):
@minrk

minrk Sep 5, 2013

Owner

same, leave as Exporter?

Owner

jdfreder commented Sep 8, 2013

@Carreau review?

Owner

Carreau commented Sep 8, 2013

Yes, slowly coming back from holidays, and caching up, but as always PhD first, and as my boss is back too, getting up to speed is not easy.

Owner

Carreau commented Sep 12, 2013

Need a rebase that I tried to force push on #3747 but github see the wrong commits after I reopened the PR...

Owner

jdfreder commented Sep 12, 2013

@Carreau , rebased

Owner

Carreau commented Sep 13, 2013

+1

Owner

jdfreder commented Sep 13, 2013

Sweet, unless there are objections, I'll merge sometime tomorrow, @minrk ?

Owner

minrk commented Sep 13, 2013

@jdfreder do not merge yourself, let someone else merge.

Owner

jdfreder commented Sep 13, 2013

let someone else merge.

If you could, that would be great

@minrk minrk commented on an outdated diff Sep 13, 2013

IPython/nbconvert/exporters/exporter.py
- template_file = Unicode(u'default',
- config=True,
- help="Name of the template file to use")
- def _template_file_changed(self, name, old, new):
- if new=='default':
- self.template_file = self.default_template
- else:
- self.template_file = new
- self.template = None
- self._load_template()
-
- default_template = Unicode(u'')
- template = Any()
- environment = Any()
+ # finish the docstring
@minrk

minrk Sep 13, 2013

Owner

has a todo mark for finishing the docstring, do you want to finish it?

@minrk minrk commented on an outdated diff Sep 13, 2013

IPython/nbconvert/exporters/exporter.py
"""
nb_copy = copy.deepcopy(nb)
resources = self._init_resources(resources)
# Preprocess
- nb_copy, resources = self._preprocess(nb_copy, resources)
+ nb_copy, resources = self._transform(nb_copy, resources)
@minrk

minrk Sep 13, 2013

Owner

this should be preprocess, not transform, right?

@minrk minrk commented on the diff Sep 13, 2013

IPython/nbconvert/exporters/exporter.py
with io.open(filename) as f:
- return self.from_notebook_node(nbformat.read(f, 'json'), resources=resources,**kw)
+ return self.from_notebook_node(nbformat.read(f, 'json'), resources=resources, **kw)
@minrk

minrk Sep 13, 2013

Owner

you removed **kw from the signature above, so if kw is not empty, this is broken.

@minrk minrk commented on an outdated diff Sep 13, 2013

IPython/nbconvert/exporters/exporter.py
@@ -305,11 +169,11 @@ def from_file(self, file_stream, resources=None, **kw):
def register_preprocessor(self, preprocessor, enabled=False):
"""
Register a preprocessor.
- Preprocessors are classes that act upon the notebook before it is
- passed into the Jinja templating engine. Preprocessors are also
+ preprocessors are classes that act upon the notebook before it is
+ passed into the Jinja templating engine. preprocessors are also
@minrk

minrk Sep 13, 2013

Owner

Presumably an overzealous find/replace for capitalization - these are sentences.

@minrk minrk commented on an outdated diff Sep 13, 2013

IPython/nbconvert/exporters/exporter.py
- def _preprocess(self, nb, resources):
+
+ def _transform(self, nb, resources):
@minrk

minrk Sep 13, 2013

Owner

should be preprocess, not transform, right?

Owner

minrk commented Sep 13, 2013

Looks like the rebase didn't quite go as planned - there look to be a few find/replace errors and remaining calls to transform. Also don't forget to add at least a simple exercise test that goes through the motions of the nb2nb Exporter.

Owner

jdfreder commented Sep 13, 2013

Looks like the rebase didn't quite go as planned

Apparently, gah! 😦 I wonder if that happened the second rebase around? It didn't ask for my intervention, but it did complain about some files.

Anyways, I addressed your comments, thanks, and I added a few small tests.

@minrk minrk added a commit that referenced this pull request Sep 18, 2013

@minrk minrk Merge pull request #4175 from jdfreder/exporterbase
add base Exporter class above TemplateExporter
197a4bf

@minrk minrk merged commit 197a4bf into ipython:master Sep 18, 2013

1 check passed

default The Travis CI build passed
Details

jdfreder deleted the jdfreder:exporterbase branch Mar 10, 2014

@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

@minrk minrk Merge pull request #4175 from jdfreder/exporterbase
add base Exporter class above TemplateExporter
4940060
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment