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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Inline expression parsing #1056

Merged
merged 1 commit into from Apr 4, 2024
Merged

馃悰 Inline expression parsing #1056

merged 1 commit into from Apr 4, 2024

Conversation

rowanc1
Copy link
Member

@rowanc1 rowanc1 commented Apr 4, 2024

Fixes a bug where the inline expressions were not getting picked up properly in the mystmd side when they were previously executed in JupyterLab-MyST.

@rowanc1 rowanc1 requested a review from fwkoch April 4, 2024 22:11
@@ -22,7 +22,7 @@ import type { IUserExpressionMetadata } from '../transforms/index.js';

function blockParent(cell: ICell, children: GenericNode[]) {
const type = cell.cell_type === CELL_TYPES.code ? NotebookCell.code : NotebookCell.content;
return { type: 'block', meta: JSON.stringify({ type, ...cell.metadata }), children };
return { type: 'block', data: copyNode({ type, ...cell.metadata }), children };
Copy link
Member Author

Choose a reason for hiding this comment

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

The meta --> data happens in a transform but the inline expressions happen before that transform is run. I think that storing things in meta is a relic of how we used to parse based on strings, and I think we may be able to drop that part. @fwkoch can you confirm?

Copy link
Member

@fwkoch fwkoch Apr 4, 2024

Choose a reason for hiding this comment

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

Yeah, I think this is fine to do here. meta -> data transform keeps anything that already exists in data, so putting things in data earlier is ok.

We can't totally drop the meta support yet, I don't think, because it's still used in myst-parser for keeping track of content on blockBreaks (which will be converted to blocks later) https://github.com/executablebooks/mystmd/blob/main/packages/myst-parser/src/tokensToMyst.ts#L468

But... we could probably do a JSON.parse there instead, e.g.

  myst_block_break: {
    type: 'blockBreak',
    noCloseToken: true,
    isLeaf: true,
    getAttrs(t) {
      return {
        data: JSON.parse(t.content) || undefined,
      };
    },

then we could maybe get rid of the meta transform entirely...?

There's still some other vestiges of meta we would need to clean up, e.g. https://github.com/executablebooks/mystmd/blob/main/packages/myst-to-md/src/misc.ts#L14 (but that one probably doesn't work anyway as is, since meta is deleted after transforming to data...)

@rowanc1
Copy link
Member Author

rowanc1 commented Apr 4, 2024

Merging this in, @fwkoch interested in your thoughts on if we should remove the .meta = JSON.stringify() logic and tests now that we are no longer parsing by reading in text.

@rowanc1 rowanc1 merged commit 49fd6b9 into main Apr 4, 2024
4 checks passed
@rowanc1 rowanc1 deleted the feat/inline-expressions branch April 4, 2024 22:31
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.

None yet

2 participants