-
Notifications
You must be signed in to change notification settings - Fork 572
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
Don't remove empty cells by default #784
Conversation
8595f56
to
308fb29
Compare
Ping? |
1 similar comment
Ping? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to merge this to have more canonical default behaviour.
|
||
> jupyter nbconvert --RegexRemovePreprocessor.enabled=True \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value for the enabled
flag appears to be False
. We should either override the default value for the RegexRemovePreprocessor
or keep the --RegexRemovePreprocessor.enabled=True
flag. Or is a preprocessor enabled automatically if one of its attributes (patterns
) is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tillahoffmann Thanks for the review!
I'm not quite sure, but I think the preprocessor is enabled by default because of this:
nbconvert/nbconvert/exporters/templateexporter.py
Lines 126 to 135 in 9f5c402
@property | |
def default_config(self): | |
c = Config({ | |
'RegexRemovePreprocessor': { | |
'enabled': True | |
}, | |
'TagRemovePreprocessor': { | |
'enabled': True | |
} | |
}) |
I don't care if it is enabled or not, as long as it doesn't remove stuff by default.
modify the patterns traitlet. For example, execute the following command | ||
to convert a notebook to html and remove cells containing only whitespace: | ||
to convert a notebook to html and remove cells containing only whitespace:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typo of an extra :
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually reST syntax, see http://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#literal-blocks
Without that, the formatting doesn't work, see the currently broken docs:
https://nbconvert.readthedocs.io/en/5.3.1/api/preprocessors.html#nbconvert.preprocessors.RegexRemovePreprocessor
The first command line argument enables the preprocessor and the second | ||
sets the list of patterns to '\\s*\\Z' which matches an arbitrary number | ||
The command line argument | ||
sets the list of patterns to ``'\\s*\\Z'`` which matches an arbitrary number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are single back ticks sufficient here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it, and apparently not.
Sphinx allows to customize the formatting of single backticks, but it is apparently not configured for typewriter text (and I also don't think it would be a good idea to do so).
@tillahoffmann Is there anything else you need me to do? |
Happy for this PR to be merged. As an aside, thank you for the detailed Sphinx explanation. |
Thanks! |
Since there was no real response to #720, I thought I'll try it with a pull request ...
This PR restores the sane behavior of not removing stuff without the user wanting to remove stuff.
It also fixes the docstring so if people actually want to remove stuff, they can learn how to do that.