-
Notifications
You must be signed in to change notification settings - Fork 496
Serialize MarkdownAST nodes via Markdown.jl when getting hash for admonitions #2710
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Michael Goerz <[email protected]>
It probably would work, but internally |
Oops - will look into that at some point. An MWE is probably not too hard to create on the MarkdownAST end from the tree you posted. |
Turns out to be not too bad to avoid the bad methods, just undocumented. This version of the function fixes the build of Base docs for me: diff --git a/src/html/HTMLWriter.jl b/src/html/HTMLWriter.jl
index 21cb8f6d1..05abd6c0b 100644
--- a/src/html/HTMLWriter.jl
+++ b/src/html/HTMLWriter.jl
@@ -2369,6 +2369,20 @@ function domify(dctx::DCtx, node::Node, f::MarkdownAST.FootnoteDefinition)
return DOM.Node[]
end
+# This function inspired by Michael Goerz in https://github.com/JuliaDocs/MarkdownAST.jl/issues/18
+function _markdownast_to_str(node::MarkdownAST.Node)
+ md = MarkdownAST._convert_element(node)
+ text = Markdown.plain(md)
+ return strip(text)
+end
+# uncomment the following to debug mystery hangs:
+#MarkdownAST._convert_element(::MarkdownAST.Node, e::MarkdownAST.AbstractElement) =
+# error("Unable to convert element of type $(typeof(e))")
+MarkdownAST._convert_element(::MarkdownAST.Node, e::Documenter.AbstractDocumenterBlock) = "TODO"
+MarkdownAST._convert_element(::MarkdownAST.Node, e::Documenter.PageLink) = "TODO"
+MarkdownAST._convert_element(::MarkdownAST.Node, e::Documenter.LocalLink) = "TODO"
+MarkdownAST._convert_element(::MarkdownAST.Node, e::Documenter.LocalImage) = "TODO"
+
function domify(dctx::DCtx, node::Node, a::MarkdownAST.Admonition)
@tags header div details summary
colorclass = |
Note that I only posted a fraction of the tree. The dump of the actual tree was 15k lines long, when printed even just with a limited depth |
FWIW, that also points to the source of the failure also: there is no show method for |
@vtjnash I'm confused by this. As for the bug itself -- I think the correct solution here is to add the necessary |
I have the impression now that |
I was looking at the source before making that claim: all of the native elements have all their fields declared mutable: https://github.com/JuliaDocs/MarkdownAST.jl/blob/main/src/markdown.jl except for the non-native element But you're right: I thought the API said you could mutate elements in place, but it may have meant the field, not the values. And I thought you'd need to implement |
That's fair actually. But yea, the intent there is two fold: (1) you should be able to change the But there is no hard requirement for the
Hmm. Actually, maybe, yes, it would make sense (and be safer?). And potentially produce more stable hashes (it wouldn't be dependent on internal Julia representation etc). But it shouldn't be strictly necessary for the bugfix, so let's leave that as a change for the next minor release. |
This was an online only PR so it might fail CI :D
But the idea here is to do a best effort render to Markdown.jl, assuming all the crazy magic has already been expanded by ExpandTemplates. This might potentially help with issues hashing admonition nodes as reported in #2676 (comment).
Still needs a changelog and tests to make sure this doesn't happen again, but let's see if this approach is viable first