📥 Fix embedding raw code with remove-input flag
#1937
Conversation
🦋 Changeset detectedLatest commit: 819191e The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
agoose77
changed the title
remove-input flagremove-input flag
There was a problem hiding this comment.
I added some brief documentation to clarify this for future reference.
This looks fine to merge to me, assuming the logic you're following to detect a notebook cell is correct. My one suggestion would be to add a test to make sure that this is behaving as expected, but I don't think it's strictly required if you are confident that this fixes the bug.
| return !( | ||
| n.type === 'code' && | ||
| parent?.type === 'block' && | ||
| parent?.kind === NotebookCell.code | ||
| ); |
There was a problem hiding this comment.
The previous logic had a special case for code-nodes that were tagged as outputs. Now, we want to be even stricter --- only code nodes that are children of code-cells and not tagged as outputs.
| // should be removed | ||
| return !( | ||
| n.type === 'code' && | ||
| n.data?.type !== 'output' && |
There was a problem hiding this comment.
@fwkoch I don't think I need this test for n.data.type actually, because reduceOutputs is always called after this transform runs. Shouldn't this mean we'll never see reduced outputs here?
There was a problem hiding this comment.
I think you are correct that reduceOutputs always comes after this. However, I think we should just leave this check in place. At worst, it's just a no-op, but, at best, it would allow tagging nodes as an output in contexts other than the standard Jupyter Notebook processing (e.g. a custom transform plugin).
| @@ -35,13 +36,28 @@ function mutateEmbedNode( | |||
| const { url, dataUrl, targetFile, sourceFile } = opts ?? {}; | |||
| if (targetNode && node['remove-output']) { | |||
| targetNode = filter(targetNode, (n: GenericNode) => { | |||
| // After reduction, 'output' nodes may be replaced by their children which are then tagged as outputs | |||
| return n.type !== 'output' && n.data?.type !== 'output'; | |||
There was a problem hiding this comment.
See lower comment about reduceOutputs
| "exports": [ | ||
| { | ||
| "format": "md", | ||
| "filename": "index.md", | ||
| "url": "/index-52e2bc751b3bb2cb9ed25191a3951cf9.md" | ||
| } | ||
| ] |
There was a problem hiding this comment.
Auxiliary test changes
agoose77
force-pushed
the
agoose77/fix-embed-code
branch
from
69f24df to
f8cb8ec
Compare
|
@fwkoch I am not 100% clear on the When you get a moment, could you take this over the line and add in-line comments to explain for future reference? |
agoose77
moved this from In Progress
to Blocked on review
in Jupyter Book and MyST Team Priorities
fwkoch
force-pushed
the
agoose77/fix-embed-code
branch
from
f8cb8ec to
819191e
Compare
There was a problem hiding this comment.
Ok, finally took the time to understand the purpose of this PR and what it is fixing. The scope is actually quite small, simple, and safe; it just makes remove-input a bit stricter so it's actually "remove input" rather than "remove code." Easy to reproduce and see the improvement.
@agoose77 - Regarding your question about data.input, I don't see this anywhere, either in this PR or in reduceOutputs or anything, so I don't think we need to worry about it? We could add it in as a way for users to tag specific, non-code nodes as input? But that's a separate feature. Regarding output tags - I made a comment, but I think it doesn't hurt to just leave as-is.
Also, I noticed an edge case - if you try to embed a notebook code cell using  that has not been executed (i.e. just input, no output) it will still be removed, leaving nothing. I think this is fine, though, especially with the sentence added to the docs explicitly stating notebook input will be removed.
Sorry this review took 3 months 😬
| @@ -60,7 +60,9 @@ For example, the following references the admonitions list in [](admonitions.md) | |||
|
|
|||
| ### The `` short-hand | |||
|
|
|||
| The embedding markdown shorthand lets you quickly embed content using the Markdown image syntax (see more about [images](./figures.md)). | |||
| The embedding shorthand lets you embed content using the Markdown image syntax (see more about [images](./figures.md)). | |||
| This **removes the input cell** if you are [embedding from a Jupyter notebook](./reuse-jupyter-outputs.md). | |||
| // should be removed | ||
| return !( | ||
| n.type === 'code' && | ||
| n.data?.type !== 'output' && |
There was a problem hiding this comment.
I think you are correct that reduceOutputs always comes after this. However, I think we should just leave this check in place. At worst, it's just a no-op, but, at best, it would allow tagging nodes as an output in contexts other than the standard Jupyter Notebook processing (e.g. a custom transform plugin).
github-project-automation
bot
moved this to Done
in Jupyter Book and MyST Team Priorities
Fixes #1932