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

Remove Nbconvert template loading magic #4727

Merged
merged 9 commits into from Jan 8, 2014

Conversation

takluyver
Copy link
Member

This specifies the template names in the exporter class, rather than getting them from the module names, which seemed a bit too magical. This continues #4724, which I made from my master branch by accident. The problems @damianavila noted there are fixed.

Also, the latex templates got renamed from latex/latex_foo.tplx to latex/foo.tplx.

At present, when specifying a template to use, you have to specify the full template name, e.g. html_basic, not just basic. We may want to think more about this.

@damianavila
Copy link
Member

LGTM now... thanks!

At present, when specifying a template to use, you have to specify the full template name, e.g. html_basic, not just basic. We may want to think more about this.

Regarding this one... I don't have strong feelings about this issue but I think I am +0 for the full name: for people using the command line UI, I think that a more explicit name as html_basic or html_full is more informative about what they are asking for...

@minrk
Copy link
Member

minrk commented Dec 27, 2013

I think it's a little unfortunate to add back the redundancy of --to html --template html_basic, but the explicitness of the code is definitely an improvement. @ellisonbg @Carreau?

@Carreau
Copy link
Member

Carreau commented Dec 27, 2013

I think it is ok to make the command line tool a little more complicated,
for 1.0 nbconvert was ment to be a technical preview, I'm not sure it will
be stable for 2.0. As long as the library improve then it is easier to
build command line wrapper around it. Also I was never a big fan of --to
[format] anyway. I'm also considering moving more toward dotted object name
in the long run so that peoples could install full blow templates with pip
and refer to them by Package.templatename.

On Fri, Dec 27, 2013 at 8:42 PM, Min RK notifications@github.com wrote:

I think it's a little unfortunate to add back the redundancy of --to html
--template html_basic, but the explicitness of the code is definitely an
improvement. @ellisonbg https://github.com/ellisonbg @Carreauhttps://github.com/Carreau
?


Reply to this email directly or view it on GitHubhttps://github.com//pull/4727#issuecomment-31276886
.

@damianavila
Copy link
Member

I'm also considering moving more toward dotted object name
in the long run so that peoples could install full blow templates with pip
and refer to them by Package.templatename.

This is a very interesting idea...

@takluyver
Copy link
Member Author

Discussed this with Min, and we've put the HTML templates in a separate directory, like the latex templates already were. We can follow suit with any exporter that has more than one possible template. The exporter defines a default_template_directory, so you can once again do --to html --template basic.

We discussed making default_template_directory a list, so it can look for templates in multiple places (e.g. if there were multiple slides templates, but they inherit from the HTML templates, you'd want both directories listed). But we decided to leave that for another day.

os.path.join("..", "templates", "latex"), config=True,
help="Path where the template files are located.")
def _default_template_path_default(self):
return os.path.join("..", "templates", "latex")

template_skeleton_path = Unicode(
Copy link
Member

Choose a reason for hiding this comment

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

same treatment for template_skeleton_path as default_template_path, probably

@takluyver
Copy link
Member Author

@minrk done & done.

@takluyver
Copy link
Member Author

Weird, it works for me when I run setup.py install locally. Anyway, I tweaked it to *.*.

@takluyver
Copy link
Member Author

And Travis is happy now.

@minrk
Copy link
Member

minrk commented Jan 8, 2014

hooray!

minrk added a commit that referenced this pull request Jan 8, 2014
Remove Nbconvert template loading magic based on module name.

A little more explicit now, plus a few traitlets cleaned up.
@minrk minrk merged commit 902758e into ipython:master Jan 8, 2014
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Remove Nbconvert template loading magic based on module name.

A little more explicit now, plus a few traitlets cleaned up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants