Skip to content

🎨 Markdown formatting improvements #2245

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.

Sign up for GitHub

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

Draft
rowanc1 wants to merge 1 commit into
base: main
Choose a base branch
from
+501 −171
Draft

🎨 Markdown formatting improvements #2245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/cool-ducks-sing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"myst-parser": patch
---

Add body/options to the mystDirective and mystRole nodes
5 changes: 5 additions & 0 deletions .changeset/tender-pianos-cheat.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"myst-to-md": patch
---

Improve formatting of raw myst-directives
1 change: 1 addition & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 8 additions & 8 deletions packages/myst-ext-card/tests/card.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ describe('card directive', () => {
{
type: 'mystDirective',
name: 'card',
args: 'Card Title',
args: [{ type: 'text', value: 'Card Title' }],
value: 'Header\n^^^\n\nCard content\n+++\nFooter',
position: {
start: {
Expand Down Expand Up @@ -142,7 +142,7 @@ describe('card directive', () => {
const output = mystParse(content, {
directives: [cardDirective],
});
expect(output).toEqual(expected);
expect(output).toMatchObject(expected);
Copy link
Member Author

@rowanc1 rowanc1 Aug 12, 2025

Choose a reason for hiding this comment

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

Tests in the extension code are made a bit less strict here. No changes to the code, so no need to bump the packages.

});
it('card directive parses with options', async () => {
const content =
Expand All @@ -153,11 +153,11 @@ describe('card directive', () => {
{
type: 'mystDirective',
name: 'card',
args: 'Card Title',
args: [{ type: 'text', value: 'Card Title' }],
options: {
header: 'Header',
footer: 'Footer',
link: 'my-url',
header: [{ type: 'text', value: 'Header' }],
footer: [{ type: 'text', value: 'Footer' }],
url: 'my-url',
},
value: 'Card\n^^^\ncontent',
position: {
Expand Down Expand Up @@ -280,7 +280,7 @@ describe('card directive', () => {
const output = mystParse(content, {
directives: [cardDirective],
});
expect(output).toEqual(expected);
expect(output).toMatchObject(expected);
});
it('card directive parses with minimal content', async () => {
const content = '```{card}\nCard content\n```';
Expand Down Expand Up @@ -343,7 +343,7 @@ describe('card directive', () => {
const output = mystParse(content, {
directives: [cardDirective],
});
expect(output).toEqual(expected);
expect(output).toMatchObject(expected);
});
});

Expand Down
12 changes: 6 additions & 6 deletions packages/myst-ext-exercise/tests/exercise.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe('exercise directive', () => {
options: {
label: 'ex-1',
},
args: 'Exercise Title',
args: [{ type: 'text', value: 'Exercise Title' }],
value: 'Exercise content',
children: [
{
Expand Down Expand Up @@ -58,7 +58,7 @@ describe('exercise directive', () => {
const output = mystParse(content, {
directives: [exerciseDirective],
});
expect(deletePositions(output)).toEqual(expected);
expect(deletePositions(output)).toMatchObject(expected);
});
it('nonumber is prioritized over enumerated', async () => {
const content =
Expand All @@ -74,7 +74,7 @@ describe('exercise directive', () => {
enumerated: true,
nonumber: true,
},
args: 'Exercise Title',
args: [{ type: 'text', value: 'Exercise Title' }],
value: 'Exercise content',
children: [
{
Expand Down Expand Up @@ -110,7 +110,7 @@ describe('exercise directive', () => {
const output = mystParse(content, {
directives: [exerciseDirective],
});
expect(deletePositions(output)).toEqual(expected);
expect(deletePositions(output)).toMatchObject(expected);
});
it('exercises are enumerated with labels by default', async () => {
const content = '```{exercise} Exercise Title\nExercise content\n```';
Expand All @@ -125,7 +125,7 @@ describe('exercise directive', () => {
{
type: 'mystDirective',
name: 'exercise',
args: 'Exercise Title',
args: [{ type: 'text', value: 'Exercise Title' }],
value: 'Exercise content',
children: [
{
Expand Down Expand Up @@ -158,6 +158,6 @@ describe('exercise directive', () => {
},
],
};
expect(deletePositions(output)).toEqual(expected);
expect(deletePositions(output)).toMatchObject(expected);
});
});
2 changes: 1 addition & 1 deletion packages/myst-ext-grid/tests/grid.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,6 @@ describe('grid directive', () => {
const output = mystParse(content, {
directives: [gridDirective],
});
expect(output).toEqual(expected);
expect(output).toMatchObject(expected);
});
});
4 changes: 2 additions & 2 deletions packages/myst-ext-icon/tests/icon.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe('icon role', () => {
const output = mystParse(markup, {
roles: [iconRole],
});
expect(deletePositions(output)).toEqual(expected);
expect(deletePositions(output)).toMatchObject(expected);
},
);
it.each(Object.entries(LEGACY_ICON_ALIASES))(
Expand Down Expand Up @@ -79,7 +79,7 @@ describe('icon role', () => {
const output = mystParse(markup, {
roles: [iconRole],
});
expect(deletePositions(output)).toEqual(expected);
expect(deletePositions(output)).toMatchObject(expected);
},
);
});
4 changes: 2 additions & 2 deletions packages/myst-ext-proof/tests/proof.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ describe('proof directive', () => {
{
type: 'mystDirective',
name: 'prf:proof',
args: 'Proof Title',
args: [{ type: 'text', value: 'Proof Title' }],
value: 'Proof content',
position: {
start: {
Expand Down Expand Up @@ -86,6 +86,6 @@ describe('proof directive', () => {
const output = mystParse(content, {
directives: [proofDirective],
});
expect(output).toEqual(expected);
expect(output).toMatchObject(expected);
});
});
4 changes: 2 additions & 2 deletions packages/myst-ext-reactive/tests/reactive.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe('reactive tests', () => {
roles: [reactiveRole],
directives: [reactiveDirective],
});
expect(output).toEqual(expected);
expect(output).toMatchObject(expected);
});
it('r:dynamic role parses', async () => {
const content = '{r:dynamic}`rValue="visitors", rChange="{visitors: value}", value="5"`';
Expand Down Expand Up @@ -92,6 +92,6 @@ describe('reactive tests', () => {
roles: [reactiveRole],
directives: [reactiveDirective],
});
expect(output).toEqual(expected);
expect(output).toMatchObject(expected);
});
});
10 changes: 5 additions & 5 deletions packages/myst-ext-tabs/tests/tabs.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe('tab directives', () => {
const output = mystParse(content, {
directives: [...tabDirectives],
});
expect(deletePositions(output)).toEqual(expected);
expect(deletePositions(output)).toMatchObject(expected);
});
it('tabSet class option parses', async () => {
const content = '```{tab-set}\n:class: my-class\n```';
Expand All @@ -57,7 +57,7 @@ describe('tab directives', () => {
const output = mystParse(content, {
directives: [...tabDirectives],
});
expect(deletePositions(output)).toEqual(expected);
expect(deletePositions(output)).toMatchObject(expected);
});
it.each(['tab-item', 'tabItem'])('%s with title parses', async (name: string) => {
const content = `\`\`\`{${name}} Tab One\n\`\`\``;
Expand All @@ -81,7 +81,7 @@ describe('tab directives', () => {
const output = mystParse(content, {
directives: [...tabDirectives],
});
expect(deletePositions(output)).toEqual(expected);
expect(deletePositions(output)).toMatchObject(expected);
});
it('tabItem sync and selected options parse', async () => {
const content = '```{tab-item} Tab One\n:sync: tab1\n:selected:\n```';
Expand Down Expand Up @@ -111,7 +111,7 @@ describe('tab directives', () => {
const output = mystParse(content, {
directives: [...tabDirectives],
});
expect(deletePositions(output)).toEqual(expected);
expect(deletePositions(output)).toMatchObject(expected);
});
// TODO: enable when we have a better required/fallback/default pattern
it.skip('tabItem without title errors', async () => {
Expand Down Expand Up @@ -202,6 +202,6 @@ describe('tab directives', () => {
const output = mystParse(content, {
directives: [...tabDirectives],
});
expect(deletePositions(output)).toEqual(expected);
expect(deletePositions(output)).toMatchObject(expected);
});
});
4 changes: 3 additions & 1 deletion packages/myst-parser/src/directives.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import type {
DirectiveData,
DirectiveSpec,
DirectiveContext,
ParseTypes,
GenericParent,
} from 'myst-common';
import { RuleId, fileError, fileWarn } from 'myst-common';
Expand Down Expand Up @@ -94,6 +93,7 @@ export function applyDirectives(
if (argSpec.required && data.arg == null) {
validationError = true;
}
node.args = data.arg;
Copy link
Member Author

@rowanc1 rowanc1 Aug 12, 2025

Choose a reason for hiding this comment

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

This is the biggest change is that we put the arg, options and body of roles and directives onto the mystDirective node. This allows us to process them afterward in a formatting package - or transform a figure into a directive etc. rather than doing that in the markdown writer.

This shouldn't matter for the duplication as we remove these nodes early in the processing for most things. But it is really helpful to have them for the formatting / writing of markdown.

We might want to change it to node.argsParsed or something so we have the pre/post parsing? Or put it on the data field.

@fwkoch or @agoose77 if you have opinions here, that would help.

Copy link
Member

@fwkoch fwkoch Aug 14, 2025

Choose a reason for hiding this comment

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

Ok - seeing this, my first impression was "we already stash this stuff on the mystDirective nodes, don't we...?" - That's not entirely true.

On the initial parse pass, we do save args and options to the mystDirective node; these are the raw, unparsed values. I don't think these fields are used anywhere. Additionally, we create mystDirectiveArg/Option/Body nodes, where the raw value is saved to be parsed, in this applyDirectives function. Within this function we are building a data object of type DirectiveData which contains all the parsed arg/option/body content. Critically, this data object is transient; it's just passed directly to the directive plugins.

So, the change in this PR is taking the parsed, previously transient arg/option/body values and saving them directly to the mystDirective node. I agree this is a good change.

Getting back to your specific question @rowanc1 - are we putting this in the right place? The biggest issue is that we are clobbering raw arg/option values with parsed arg/option values. I think this is fine; we really should only ever be dealing with the parsed values. The only argument I can think of for keeping the raw values is for round-tripping documents exactly as written. But that's not useful at all - MyST does not care about this (I don't think!), we just don't want to lose any data, and for that, parsed structured data is better. (This is especially true if we are thinking about converting between formats - in that case, any raw value is totally useless.)

I guess the other argument against clobbering the raw values with parsed values is simply backwards compatibility. This is a backwards incompatible change that I don't think will impact anyone, but there could be something somewhere using these raw mystDirective fields. (Maybe we make this a minor version bump?)

Regarding the alternatives you suggest:

  1. I don't think we should do argsParsed... that feels excessive; we are keeping track if node.processed === false, so if that's not the case (i.e. the node is processed), args will be parsed. It's also a little silly with bodyParsed since we are not saving the raw body...
  2. Stashing args/options/body on node.data would be fine - this is just taking the DirectiveData object and saving it as "directive data." I somewhat like that, but at the same time, the data field is usually used for metadata - in the case of mystDirective nodes, args/options/body are the fundamental content, not metadata. 🤷

So maybe it's just fine as-implemented? (We could potentially even stop creating the data object, and pass the node as DirectiveData to the run function... But I think for now this is overthinking it...)

TL;DR: 👍 to your change, maybe minor version bump.

}
} else if (argNode) {
const message = `unexpected argument provided for directive: ${name}`;
Expand All @@ -104,6 +104,7 @@ export function applyDirectives(
// const options: Record<string, ParseTypes> = {};
const { valid: validOptions, options } = parseOptions(name, node, vfile, optionsSpec);
data.options = options;
node.options = options;
validationError = validationError || validOptions;

// Handle body
Expand All @@ -124,6 +125,7 @@ export function applyDirectives(
`body of directive: ${name}`,
RuleId.directiveBodyCorrect,
);
node.body = data.body;
if (bodySpec.required && data.body == null) {
validationError = true;
}
Expand Down
1 change: 1 addition & 0 deletions packages/myst-parser/src/roles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export function applyRoles(tree: GenericParent, specs: RoleSpec[], vfile: VFile)
`body of role: ${name}`,
RuleId.roleBodyCorrect,
);
node.body = data.body;
if (body.required && data.body == null) {
validationError = true;
}
Expand Down
3 changes: 2 additions & 1 deletion packages/myst-to-md/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"myst-frontmatter": "^1.7.6",
"unist-util-select": "^4.0.3",
"vfile": "^5.3.7",
"vfile-reporter": "^7.0.4"
"vfile-reporter": "^7.0.4",
"longest-streak": "^3.1.0"
}
}
55 changes: 48 additions & 7 deletions packages/myst-to-md/src/directives.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import { toText, fileError, RuleId } from 'myst-common';
import { select, selectAll } from 'unist-util-select';
import type { VFile } from 'vfile';
import { longestStreak } from 'longest-streak';
import type { NestedState, Parent, Validator } from './types.js';
import { incrementNestedLevel, popNestedLevel } from './utils.js';

Expand Down Expand Up @@ -74,12 +75,45 @@
}

/**
* Generic MyST directive handler
*
* This uses the directive name/args/value and ignores any children nodes
* Handler for a raw directive
*/
function mystDirective(node: any, _: Parent, state: NestedState, info: Info): string {
return writeStaticDirective(node.name, { argsKey: 'args' })(node, _, state, info);
function writeDirective(options?: DirectiveOptions) {

Check warning on line 80 in packages/myst-to-md/src/directives.ts

View workflow job for this annotation

GitHub Actions / lint 

'options' is defined but never used
Copy link
Member

@fwkoch fwkoch Aug 14, 2025

Choose a reason for hiding this comment

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

Ok, I'm feeling a little confused about this.

First, I don't think we need this wrapper "factory function" - options is unused. We can just define the function directly.

Second, we are only using writeDirective when we encounter mystDirective nodes. These only persist in the tree when they have an unknown directive type. In that case, in the applyDirectives function, we are deleting the children and writing the options back to the body. It never even gets to the new code where node.args/node.options/node.body are set. For known directives, the mystDirective nodes get lifted out.

return (node: any, _: Parent, state: NestedState, info: Info): string => {
incrementNestedLevel('directive', state);
const { label, class: className, ...otherOptions } = node.options ?? {};
const optLabel = label ? `#${label}` : '';
const optClass = className
? className
.split(' ')
.filter((c: string) => c.trim() !== '')
.map((c: string) => `.${c.trim()}`)
.join(' ')
: '';
const optOther = Object.entries(otherOptions)
.map(([key, value]) => `${key}=${value}`)
Copy link
Member

@fwkoch fwkoch Aug 14, 2025

Choose a reason for hiding this comment

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

Writing all options this way feels risky. Should we check if value has spaces?

.join(' ');
const directiveOpts = [optLabel, optClass, optOther].filter(Boolean).join(' ');

// If the directive has a body which is an array, use it as content.
// Otherwise, use the value.
const markdownBody = node.body && Array.isArray(node.body);
const content = markdownBody
? state.containerFlow({ type: 'root', children: node.body }, info)
: node.value;

const nesting = popNestedLevel('directive', state);
const markerChar = markdownBody ? ':' : '`';
const markerLength = Math.max(longestStreak(content, markerChar) + 1, nesting + 3);
const marker = markerChar.repeat(markerLength);
const directiveInline = [node.name, directiveOpts].filter(Boolean).join(' ');
const args = Array.isArray(node.args)
? state.containerPhrasing({ type: 'heading', depth: 1, children: node.args }, info)
Copy link
Member

@fwkoch fwkoch Aug 14, 2025

Choose a reason for hiding this comment

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

I don't get this - Are we injecting this heading just to prepend #...? I don't think we always want to do that? But I might just be completely misunderstanding!

: node.args;
const directiveLines = [`${marker}{${directiveInline}}${args ? ' ' : ''}${args ? args : ''}`];
if (content) directiveLines.push(content);
directiveLines.push(marker);
return directiveLines.join('\n');
};
}

const CODE_BLOCK_KEYS = [
Expand Down Expand Up @@ -388,6 +422,13 @@
return writeFlowDirective(name, args, options)(nodeCopy, _, state, info);
}

function math(node: any, _: Parent, state: NestedState, info: Info): string {
if (!node.typst && !node.label) {
return `$$\n${node.value}\n$$`;
}
return writeStaticDirective('math', { keys: ['label', 'typst'] })(node, _, state, info);
}

export const directiveHandlers: Record<string, Handle> = {
code,
image,
Expand All @@ -403,11 +444,11 @@
}),
tabSet: writeFlowDirective('tab-set'),
tabItem,
math: writeStaticDirective('math', { keys: ['label'] }),
math,
embed: writeStaticDirective('embed', { argsKey: 'label' }),
include: writeStaticDirective('include', { argsKey: 'file' }),
mermaid: writeStaticDirective('mermaid'),
mystDirective,
mystDirective: writeDirective(),
};

export const directiveValidators: Record<string, Validator> = {
Expand Down
Loading
Loading