Complete ipynb export support#1915
Conversation
|
kp992
changed the title
|
@rowanc1 I have added/fixed the test cases from your PR. Should I work on capturing more metadata and output of the cells in the same PR or the follow up is okay? |
packages/myst-to-ipynb/src/index.ts
Outdated
| } | ||
|
|
||
| if (md_cells.length != 0) { | ||
| cells.push(markdownString(file, md_cells)); |
There was a problem hiding this comment.
@kp992 this is a "policy" decision to fuse Markdown cells. Is there a particular reason for doing this? With the base PR #1882, if you ingest an ipynb file and export an ipynb, then you'll end up with the same number of cells (perhaps the frontmatter might be moved, though).
If you have a use-case for fusing Markdown cells, I'd suggest adding that as an option!
There was a problem hiding this comment.
Thanks for pointing this and reviewing the PR. I was actually in a confusion splitting at \n may break the code where the string contains \n. But we do have an escape character added before that after parsing so that should not be any issue. I tried that case works fine with the splitting logic so I will put that logic back. I have also updated the tests according to that.
There was a problem hiding this comment.
I still think these changes need reverting. Consider the following Markdown notebook
---
kernelspec:
name: python3
display_name: Python 3
---
```{code-cell} python3
import sys
```
+++ { "kind": "notebook-content" }
Desc 1
+++ { "kind": "notebook-content" }
Desc 2
```{code-cell} python3
import os
```It's the MD equivalent of having four cells:
- code
- md
- md
- code
In this PR, that gets flattened to
- code
- md
- code
(and the myst-to-md also writes the block separator +++ inside the cell, which is not quite right).
Can you elaborate on why you made this particular change? Did the original PR fail to convert something properly?
I've changed the base of this PR so that it will be merged into the original PR. If the original PR changes, we'll need to rebase this one first. So, the goal should be to get this one over the line ASAP by keeping it small! The test changes are welcome — I haven't looked at them in any detail, but it would be wonderful to get the test suite passing. If you have time to address my query about the fusing of Markdown cells, we can then look at what needs to be done to close this out and open a follow-up PR if needs be! |
|
@agoose77 Can you please review it once? Thanks in advance |
packages/myst-to-ipynb/src/index.ts
Outdated
| } | ||
|
|
||
| if (md_cells.length != 0) { | ||
| cells.push(markdownString(file, md_cells)); |
There was a problem hiding this comment.
I still think these changes need reverting. Consider the following Markdown notebook
---
kernelspec:
name: python3
display_name: Python 3
---
```{code-cell} python3
import sys
```
+++ { "kind": "notebook-content" }
Desc 1
+++ { "kind": "notebook-content" }
Desc 2
```{code-cell} python3
import os
```It's the MD equivalent of having four cells:
- code
- md
- md
- code
In this PR, that gets flattened to
- code
- md
- code
(and the myst-to-md also writes the block separator +++ inside the cell, which is not quite right).
Can you elaborate on why you made this particular change? Did the original PR fail to convert something properly?
| type TestCase = { | ||
| title: string; | ||
| markdown: string; | ||
| ipynb: string; |
There was a problem hiding this comment.
We probably don't care about the exact text representation of the ipynb file on disk. Could you change the ipynb entry to be a Record<string, any>?
Then, you'd need to update the tests to be
- title: link reference
mdast:
type: root
children:
- type: text
value: markdown
ipynb:
cells:
- instead of
- title: link reference
mdast:
type: root
children:
- type: text
value: markdown
ipynb: |-
{
"cells": [This would enable us to write the regression test in YAML (less verbose) and to choose to omit parts of the output that we don't want to test (e.g. if there were timestamps)
There was a problem hiding this comment.
Thanks for the review @agoose77. I have update the code and a couple of test cases. I this looks good, I can uncomment and work on other test cases.
|
Thanks for the work here @kp992! I'm going to merge this into the other PR, and review the result there! |
2 participants
Hi,
I am trying to complete the feature in #1882. And building my first PR on top of it. I tried to add the
ipynbwriter by visiting the blocks and merging all the markdown blocks to a single cell until a code cell appears. Please review the PR.I have a small question:
How do I test it using a CLI locally for a single file? I currently do
npm run testfrommyst-to-ipynbdirectory. Is there a way I can test someast.ymlor.mdfile toipynb?I am sorry if this is a very newbie question and documented somewhere.