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

move auto id into AST #301

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tatchi
Copy link
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
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