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

Conversation

Projects
None yet
4 participants
@tillahoffmann
Copy link
Contributor

tillahoffmann commented Apr 17, 2017

This PR removes metadata associated with outputs in the ClearOutputPreprocessor.

tillahoffmann added some commits Apr 17, 2017

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Apr 18, 2017

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

This comment has been minimized.

Copy link
Member

minrk commented Apr 18, 2017

What sets ExecuteTime?

@tillahoffmann

This comment has been minimized.

Copy link
Contributor Author

tillahoffmann commented Apr 18, 2017

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

takluyver commented Apr 20, 2017

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

takluyver and others added some commits Apr 20, 2017

Upgrade pip on Travis CI
To avoid it trying to install an incompatible version of IPython on Python 2.
@tillahoffmann

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

takluyver commented Apr 20, 2017

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

This comment has been minimized.

Copy link
Member

takluyver commented Apr 20, 2017

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

@mpacer

This comment has been minimized.

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

1 check passed

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

@tillahoffmann tillahoffmann deleted the tillahoffmann:removeoutput branch Apr 22, 2017

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