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

Add option to overwrite the highlight_code filter #877

Merged
merged 15 commits into from
Oct 31, 2018
Merged

Add option to overwrite the highlight_code filter #877

merged 15 commits into from
Oct 31, 2018

Conversation

danielfrg
Copy link
Contributor

@danielfrg danielfrg commented Sep 6, 2018

Since the highlight_code filter is created on the exporter from_notebook_node function it cannot be overwritten by the filters trait.

This makes it possible to overwrite it.

Copy link
Contributor

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

Hi. Sorry no one was reviewing this for a bit. I'll try to contribute some more reviews now and then.
One thing to ask is if you could make a trivial test to prove filters get overwritten when we use this, it'd help keep the code quality up over time. Thanks!

nbconvert/exporters/html.py Outdated Show resolved Hide resolved
nbconvert/exporters/latex.py Outdated Show resolved Hide resolved
@danielfrg
Copy link
Contributor Author

Thanks for the review Mathew. I made the change you requested and added a small test.

@danielfrg
Copy link
Contributor Author

danielfrg commented Oct 30, 2018

Ok, I finally figured out why that particular test is failing. It's not about this PR it's about travis-ci installing the new ipython 7 version. Old builds used ipython 6.X.

In ipython 7 the traceback when you keyboard interrupt is:

...
<ipython-input-2-85e2e9e78df7> in <module>
...

While in ipython 6 it was (note the parenthesis):

...
<ipython-input-2-85e2e9e78df7> in <module>()
...

I can update the test notebook to be compatible with ipython 7 and pin the ipython used in testing to be >=7.

@danielfrg
Copy link
Contributor Author

Ok, I decided to not pin the ipython version because it will break the tests for Python 2. Unfortunately then it required to duplicate some test files to test in IPython 6 and 7.

I ran the tests on master locally and they are failing because of this same reason so i bet if they get executed right now on travis it will fail but this will fix it.

@MSeal
Copy link
Contributor

MSeal commented Oct 31, 2018

Awesome. Thanks for looking into the test failure as well!

@MSeal MSeal merged commit eb3b0a7 into jupyter:master Oct 31, 2018
@MSeal MSeal added this to the 5.4.1 milestone Feb 8, 2019
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