Skip to content

move auto id into AST#301

Open
tatchi wants to merge 1 commit intoocaml-community:masterfrom
tatchi:move-auto-id-ast
Open

move auto id into AST#301
tatchi wants to merge 1 commit intoocaml-community:masterfrom
tatchi:move-auto-id-ast

Conversation

@tatchi
Copy link
Copy Markdown
Collaborator

@tatchi tatchi commented Feb 10, 2023

Fixes #296

This moves the auto identifiers logic into the AST (when parsing the document). It moves the auto_identifiers parameter from the Omd.to_html function to the Omd.{of_channel,of_string} function.

I'm not sure if this API is ideal because it means that we have to parse a document twice (once with auto_identifiers set to true and once with false) if we want to print it with/without auto-identifiers.

let with_ids = Omd.of_string ~auto_identifiers:true "..." |> Omd.to_html
let without_ids = Omd.of_string ~auto_identifiers:false "..." |> Omd.to_html

Perhaps including the auto-ids in the AST and deciding whether or not to include them at the time of printing would be a better solution.

let doc = Omd.of_string "..." in
let with_ids = Omd.to_html ~auto_identifiers:true doc
let without_ids = Omd.to_html ~auto_identifiers:false doc

The only problem is that we'd need to distinguish an explicit id from an auto-generated one in the AST.

show Omd.Ctor.[ h 6 [ txt "Heading 6"; em "with emphasis!" ] ];
[%expect
{| <h6 id="heading-6with-emphasis">Heading 6<em>with emphasis!</em></h6> |}]
{| <h6>Heading 6<em>with emphasis!</em></h6> |}]
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think that's a side effect of the new implementation. Since we now add the auto-ids at the time we parse the document, there's no way to have them auto-included here.

@tatchi tatchi requested a review from shonfeder February 14, 2023 06:01
@shonfeder shonfeder removed their request for review October 17, 2024 00:11
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.

move auto generated id into AST

1 participant