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

Use "title" instead of "name" for metadata to match the notebook format #703

Merged
merged 8 commits into from Dec 30, 2017

Conversation

Projects
None yet
3 participants
@m-rossi
Copy link
Contributor

m-rossi commented Nov 11, 2017

The notebook format has a metadata field for the "title" of the notebook: https://github.com/jupyter/nbformat/blob/master/nbformat/v4/nbformat.v4.schema.json#L63
I think it should be used instead of the field "name".

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Nov 11, 2017

Changing this would effectively be breaking API, because separately developed exporters and templates can use the resources dictionary.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Nov 13, 2017

There's a version of this that can be done… specifically the html and slides templates could have this change in the vein of #672

Though the logic needs to be richer to handle the case where the title is not set… as can be seen in that PR, e.g.,:

((* block title -*))
((*- if "title" in nb.metadata *))
\title{((( nb.metadata.get("title", "") | ascii_only | escape_latex )))}
((*- else *))
\title{((( resources.metadata.name | ascii_only | escape_latex )))}
((*- endif -*))
((*- endblock title *))

@m-rossi

This comment has been minimized.

Copy link
Contributor Author

m-rossi commented Nov 13, 2017

We could add some code to ensure compatibility to metadata containing "name" (e.g. copy it to "title")? Then we could match the notebook format.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Nov 13, 2017

No… we shouldn't overwrite the current logic of name, better is to change the logic of the template itself.

As @takluyver pointed out, there is an implicit API that we should be hesitant to change, so anything that we can do that instead works entirely inside templates is going to be safer.

This problem can be addressed in the templates, so let's go with that option.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Nov 13, 2017

Re: the format, technically the name and the title are different kinds of things. We just have used the name as though it were the title in nbconvert… There is no guarantee that the title is present in a notebook, it's just an option and we shouldn't overwrite notebooks to have a title when they don't.

Here's a model of why this would go wrong:

Suppose someone ran nbconvert in-place while their notebook was named notebook_a.ipynb with no title metadata. If we were to assign the metadata as part of our run, then that notebook would now have a title field with notebook_a as the title. But what if the person then changed their notebook filename to be notebook_b.ipynb… because we assigned the metadata from before, when they exported, the title "notebook_a" would be carried around with it. The person never put that metadata there, so they might have no idea where to begin debugging to figure out why their titles don't change with their filename (the currently expected behaviour).

@m-rossi

This comment has been minimized.

Copy link
Contributor Author

m-rossi commented Nov 13, 2017

Ah, understood. I will look into the templates tomorrow.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Dec 5, 2017

This is almost correct, but it's actually in the notebook metadata itself, not in the resources metadata. So, the call would be if nb.metadata.get("title","") rather than if 'title' in resources['metadata'], as it shouldn't be there redundantly with the notebook's title metadata.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Dec 6, 2017

Also, even cleaner might be to set a variable to either of the values (using an or) and then to reference that variable in a single (non-conditional) line in the template.

That'd be cleaner for the other LaTeX version as well.

@m-rossi

This comment has been minimized.

Copy link
Contributor Author

m-rossi commented Dec 6, 2017

Ah, I mixed up those two metadata dicts. Thanks!

For your second recommendation you mean using nb.metadata.get('title', '') or resources['metadata']['name']?

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Dec 6, 2017

Yes something like set nb_title = nb.metadata.get('title', '') or resources['metadata']['name'] which you can then use in the lines that follow to simplify their display logic.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Dec 11, 2017

@m-rossi I thought we had implemented examples of that already in our templates and in searching, I'm seeing that we hadn't. Are you running into any issue implementing that logic? I'm happy to help out if you are — Jinja can be weird sometimes around logical constructs & I have some ideas if that does not work in a straightforward way.

@m-rossi

This comment has been minimized.

Copy link
Contributor Author

m-rossi commented Dec 22, 2017

Sorry for the delay, I've just added the few lines.
As you said, there is no example of a set statement in the templates. Therefore I added the set-statement at the top of the document, is this the way you want the templates to look like?

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Dec 29, 2017

I think it’d be better to have it local to the block just to keep the logic local. But it’s not a big difference. I can either merge this or wait for you to make the change, which would you prefer?

@m-rossi

This comment has been minimized.

Copy link
Contributor Author

m-rossi commented Dec 29, 2017

No problem, I've just moved the lines.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Dec 30, 2017

Beautiful! Merging! Many thanks!

@mpacer mpacer merged commit 8984d4d into jupyter:master Dec 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mpacer mpacer added this to the 5.4 milestone Feb 8, 2018

@m-rossi m-rossi deleted the m-rossi:notebooktitle branch Mar 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment