-
Notifications
You must be signed in to change notification settings - Fork 120
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
Added JupterNotebookUploadQuestion. #701
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #701 +/- ##
==========================================
+ Coverage 96.82% 96.84% +0.02%
==========================================
Files 45 45
Lines 11169 11245 +76
Branches 2058 2065 +7
==========================================
+ Hits 10814 10890 +76
Misses 294 294
Partials 61 61
Continue to review full report at Codecov.
|
|
||
(body, resources) = html_exporter.from_notebook_node(notebook) | ||
|
||
return "<div class='relate-notebook-container'>%s</div>" % body |
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 hard to review, as the code was moved. What was changed in addition to the move?
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 guess config_callback
was added. Anything else?
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 guess config_callback was added. Anything else?
Yes. Because we need a different template (built-in basic.tpl
template) as compared to what we do when we call the render_notebook_cells
method, which uses the nbconvert_template.tpl
.
I'll check if there's other changes.
:param clear_markdown: a :class:`bool` instance, indicating whether markdown | ||
cells will be ignored.. | ||
:param config: a :class:`traitlets.config.loader.Config` instance. | ||
:param config_callback: a function which further handles `config` . |
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.
Be more precise 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'll try.
@@ -70,7 +70,7 @@ | |||
"hidden": False, | |||
"listed": True, | |||
"accepts_enrollment": True, | |||
"git_source": "git://github.com/inducer/relate-sample", | |||
"git_source": "git://github.com/dzhuang/relate-sample", |
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 should be changed back before merge.
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.
Sure.
return ext, b64decode(answer_data["base64_data"]) | ||
|
||
|
||
class FileUploadQuestion(FileUploadQuestionBase): |
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 hard to review, as much of this code was moved. Could you outline the changes?
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'll do that.
Here's a pretty important question that occurred to me: Can you find/add a link in source to nbconvert's policy on whether it's designed to be safe with untrusted input? |
I'll looking into that. At least, I found the notebook have the ability to execute Javascript scripts. I don't know whether that's still the case when its converted to html. That might be an issue before we can merge this PR. |
@inducer |
There's a sanitize preprocessor, we need to make sure the preprocessor is included when we do the convert. |
Fix #690