-
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
Improved attachments +fix supporting non-unique names #980
Improved attachments +fix supporting non-unique names #980
Conversation
nbconvert/exporters/html.py
Outdated
@@ -87,7 +89,29 @@ def process_attachments(self, nb, output): | |||
'data:' + att_type + ';base64, ' + attachment[att_type]) | |||
return output | |||
|
|||
def _make_attachments_globally_unique(self, nb): | |||
nb = copy.deepcopy(nb) |
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.
Do we need to do a copy, or can we modify in place?
I think this can be done more elegantly, for backtracking, the initial start was in #780 |
I think this implementation for attachments is much cleaner, since it know about it's context (the cell), so the attachments refer to the cell's attachment, avoiding name collisions (bug exposed by one of the untitests). This also will be less error prone, since no string replacement or regex-fu is used. This approach also makes embedding images much more easy compared to this approach which is something we need for voila. @MSeal would love to see this in the next release, so we can push voila forward, happy to iterate on this quickly. |
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.
LGTM
Looking forward with a release including that fix! |
Got a few hanging PRs, then we should be set to release. Some reviews of the PRs I have open would help speed it up :) |
Via voila-dashboards/voila#16 we found out that nbconvert does not handle attachments correctly when they have the same name.
With the new attachment notebook, the old result gave:
While this fix gives:
The fix works as following:
globally_unique_{cell_index}
](attachment:image.png)
by](attachment:globally_unique_2_image.png)
The last point might lead to unexpected results if somehow the markdown source include that 'magic' string. However, that would currently already break, because
nbconvert/nbconvert/exporters/html.py
Line 85 in 1fc3968