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 output metadata in ClearOutputPreprocessor. #569

Merged
merged 15 commits into from
Apr 21, 2017
Merged

Remove output metadata in ClearOutputPreprocessor. #569

merged 15 commits into from
Apr 21, 2017

Conversation

tillahoffmann
Copy link
Contributor

This PR removes metadata associated with outputs in the ClearOutputPreprocessor.

@takluyver
Copy link
Member

This seems reasonable, though I hope this doesn't become an ever-expanding list.

@takluyver takluyver added this to the 5.2 milestone Apr 18, 2017
@minrk
Copy link
Member

minrk commented Apr 18, 2017

What sets ExecuteTime?

@tillahoffmann
Copy link
Contributor Author

ExecuteTime is set by the ExecuteTime extension. I wasn't sure about which approach to take because updating the preprocessor for each extension seems odd but ExecuteTime seems like a reasonably popular extension.

@mpacer
Copy link
Member

mpacer commented Apr 18, 2017

I'm going to suggest that we make this a configurable traitlet rather than a static list that we are providing, and then individual extensions can determine whether they want to exclude their own metadata from outputs by default or not.

@tillahoffmann if you look at https://github.com/tillahoffmann/nbconvert/blob/c73eec79199584c0663189c28a7c15139f7ec515/nbconvert/preprocessors/extractoutput.py#L38 you'll see an example of this as used by the extract output preprocessor to describe the set of outputs that are to actually be extracted.

Then I'd remove the ExecuteTime entry here and make a PR on jupyter_contrib_nbextensions (using an approach similar to that described in the ExecuteTime docs https://github.com/ipython-contrib/jupyter_contrib_nbextensions/blob/master/src/jupyter_contrib_nbextensions/nbextensions/execute_time/readme.md#options).

@mpacer
Copy link
Member

mpacer commented Apr 19, 2017

You'll need to update the test, also it'd be good to test the traitlet functionality directly (so, for example you would want to have a custom value in a test notebook and configure the traitlet instance in the test to watch for and exclude that value).

@tillahoffmann
Copy link
Contributor Author

tillahoffmann commented Apr 20, 2017

Regarding the traitlet test: The ExecutePrepreprocessor seems to configure the traitlet by just setting an attribute on the instance (https://github.com/jupyter/nbconvert/blob/master/nbconvert/preprocessors/tests/test_execute.py#L75). Is that what you had in mind or is there a better way to configure the traitlet?

The 2.7 test seems to have failed because of some version conflict with pip. Is there any way I can fix that?

@takluyver
Copy link
Member

Yep, setting the attribute is the standard way to change traitlet values.

takluyver and others added 3 commits April 20, 2017 11:20
To avoid it trying to install an incompatible version of IPython on Python 2.
@tillahoffmann
Copy link
Contributor Author

tillahoffmann commented Apr 20, 2017

Ah, seems that the tests are not failing because of pip but IPython dropped support for 2.7 in version 6 (released yesterday): https://travis-ci.org/jupyter/nbconvert/jobs/223900749#L748

@takluyver
Copy link
Member

Yup, I just merged another pull request into master to use a newer version of pip on Travis, which installs the correct version of IPython.

@takluyver
Copy link
Member

This looks OK to me, I'll give others a chance to have another look over it too.

@mpacer
Copy link
Member

mpacer commented Apr 21, 2017

This looks good to me now.

@mpacer mpacer merged commit f0760b4 into jupyter:master Apr 21, 2017
@tillahoffmann tillahoffmann deleted the removeoutput branch April 22, 2017 11:15
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.

4 participants