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

🛠 fix for empty blocks in notebooks #134

Merged
merged 4 commits into from
Jun 21, 2023
Merged

🛠 fix for empty blocks in notebooks #134

merged 4 commits into from
Jun 21, 2023

Conversation

stevejpurves
Copy link
Contributor

this caused a runtime error in notebook processing from mdast

@@ -105,7 +105,7 @@ export function notebookFromMdast(
const cell = new core.ThebeNonExecutableCell(
block.key,
notebook.id,
block.children.reduce((acc, child) => acc + '\n' + (child.value ?? ''), ''),
block.children?.reduce((acc, child) => acc + '\n' + (child.value ?? ''), ''),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the if-statement will also fall through to here. This will mean that the content is undefined?

Is that fine? I am not sure what the ThebeNonExecutableCell does in this case!

Copy link
Contributor Author

@stevejpurves stevejpurves Jun 21, 2023

Choose a reason for hiding this comment

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

nice catch -- the no children condition only occurs with empty markdown (both code and raw cells have children) cells, so we want to create the non executable cell with an empty source string in that case

@rowanc1 rowanc1 merged commit beee0d6 into main Jun 21, 2023
@rowanc1 rowanc1 deleted the fix/empty-block branch June 21, 2023 20:37
@rowanc1
Copy link
Collaborator

rowanc1 commented Jun 21, 2023

Nice -- glad that you found the source of this, makes sense I think, I will make an issue upstream to ensure that blocks have a children list always as well.

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.

2 participants