-
Notifications
You must be signed in to change notification settings - Fork 501
Added anchor links to admonition blocks for direct referencing #2676
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
Conversation
|
Won't this be rather brittle, though? I.e. if another admonition is inserted or removed before the linked on, it will silently break the link (or rather the link will end up pointing to something different than intended). |
|
Though of course that's fine for "throw away linking ", eg when sending someone in Slack a link so they find something more quickly. But for stable links eg between one part of a manual to another it'd be better if there was a way to specify an anchor name. |
|
All that said, I don't really mean to be a buzzkill, sorry for that, and thank you for working on this! |
|
you're right. now I think of it I think we can add syntax for manually assigning IDs to admonitions that need to be referenced like we are doing with headers like [Header] (#id) . do you think that's a good idea? |
absolutely not, I agree with your point of this approach being brittle, thank you for pointing that out! |
|
One super-minor style nitpick: I think it would be nice if the link icon was the same size as the existing heading icons:
It's a bit tricky with admonitions actually -- syntax-wise, there is no way to add "silent" metadata (like you can with the We already create the slug based on the admonition title, if present. So if the user gives their admonitions titles, then we're less likely to run into this issue (although I guess admonition titles could also be repetitive in some cases). And OTOH, we have similar brittleness issues with headings and docstrings. I guess one option would be to create the (fallback) slug based on a hash of the content of the admonition block. That should ensure that it would only change if the content changes, and ensure that you don't get linked to the wrong admonition. But I'd also be fine with this as is to be honest, as it's already an improvement. |
|
I have fixed the link size, if you think this is good enough that is also fine with me :) |
|
@Rahban1 Yea, that hashing seems fine! If I am not mistaken, it will hash the MarkdownAST's |
|
Yes @mortenpi you're right, it converts MarkdownAST node into a string representation and then hash it and uses the first 8 characters. I will implement it tomorrow since I am traveling today :) |
|
works fine for me locally. |
|
Sorry for the delay, but LGTM! Thank you @Rahban1! |
|
Thank you 😁 |
| end | ||
|
|
||
| node_repr = sprint(io -> show(io, node)) | ||
| content_hash = bytes2hex(SHA.sha1(node_repr))[1:8] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this has introduced a severe performance regressions. For the Oscar.jl package, building its manual went from ~40 seconds to ~680 seconds on my machine. It spends all that time computing SHA1 checksums it then doesn't need...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rats. Do you have any sense of whether it's the SHA calculation that's slow, or the rendering of the block (i.e. the sprint line)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've interrupted this via ctrl-c a few (~5?) times and each time it was in the middle of a SHA computation. Not a proof yet but quite suggestive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need crypto safety here. Perhaps a weaker but faster hash suffices. Maybe even CRC32?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, that's interesting. When working on this issue, I was testing this locally and I had realized a slight delay in build time like 5-10% increase so I thought it wouldn't be a big deal (maybe because Julia docs doesn't use admonitions that much) but I didn't realize this would have such a exponential effect on other docs. Apologies!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't Base.hash of the string sufficient?
| isempty(cat_sanitized) ? "" : ".is-category-$(cat_sanitized)" | ||
| end | ||
|
|
||
| node_repr = sprint(io -> show(io, node)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes hangs when trying to build documentation for Julia Base. Since the node object is a very complicated beast, just trying to make it into a non-formatted string appears to result in a potentially unlimited amount of text being generated.
I added a little debugging printing to get a sense of how badly this is going wrong. In succeeds a handful of times, with the last successful return being:
node_repr = "@ast MarkdownAST.Admonition(\"compat\", \"Julia 1.1\") do\n MarkdownAST.Paragraph() do\n MarkdownAST.Text(\"This function requires at least Julia 1.1.\")\n end\nend\n"
And then it fails on a node that looks something like this:
typeof(node) = MarkdownAST.Node{Nothing}
typeof.(node.children) = DataType[MarkdownAST.Node{Nothing}, MarkdownAST.Node{Nothing}, MarkdownAST.Node{Nothing}]
MarkdownAST.Node{Nothing}
t: MarkdownAST.Admonition
category: String "warning"
title: String "Warning"
parent: MarkdownAST.Node{Nothing}
t: MarkdownAST.Document MarkdownAST.Document()
parent: Nothing nothing
first_child: MarkdownAST.Node{Nothing}
t: Documenter.AnchoredHeader
anchor: Documenter.Anchor
object: MarkdownAST.Heading
level: Int64 1
order: Int64 863
file: String "/home/vtjnash/julia/doc/_build/html/en/manual/metaprogramming.md"
id: String "Metaprogramming"
nth: Int64 1
node: MarkdownAST.Node{Nothing}#= circular reference @-3 =#
parent: MarkdownAST.Node{Nothing}#= circular reference @-2 =#
first_child: MarkdownAST.Node{Nothing}
t: MarkdownAST.Heading
level: Int64 1
parent: MarkdownAST.Node{Nothing}#= circular reference @-2 =#
first_child: MarkdownAST.Node{Nothing}
t: MarkdownAST.Text
text: String "Metaprogramming"
parent: MarkdownAST.Node{Nothing}#= circular reference @-2 =#
first_child: Nothing nothing
last_child: Nothing nothing
prv: Nothing nothing
nxt: Nothing nothing
meta: Nothing nothing
last_child: MarkdownAST.Node{Nothing}
t: MarkdownAST.Text
text: String "Metaprogramming"
parent: MarkdownAST.Node{Nothing}#= circular reference @-2 =#
first_child: Nothing nothing
last_child: Nothing nothing
prv: Nothing nothing
nxt: Nothing nothing
meta: Nothing nothing
prv: Nothing nothing
nxt: Nothing nothing
meta: Nothing nothing
last_child: MarkdownAST.Node{Nothing}
t: MarkdownAST.Heading
level: Int64 1
parent: MarkdownAST.Node{Nothing}#= circular reference @-2 =#
with a stacktrace that looks something like this (repeated a dozen or so times):
...
_show_default at ./show.jl:511
show_default at ./show.jl:494 [inlined]
show at ./show.jl:489
jfptr_show_70793 at /home/vtjnash/julia1/usr/lib/julia/sys.so (unknown line)
_jl_invoke at /home/vtjnash/julia1/src/gf.c:3490 [inlined]
ijl_apply_generic at /home/vtjnash/julia1/src/gf.c:3690
_show_default at ./show.jl:511
show_default at ./show.jl:494 [inlined]
show at ./show.jl:489 [inlined]
print at ./strings/io.jl:35
print at ./strings/io.jl:46
unknown function (ip: 0x7fd0b76c385d) at (unknown file)
_jl_invoke at /home/vtjnash/julia1/src/gf.c:3490 [inlined]
ijl_apply_generic at /home/vtjnash/julia1/src/gf.c:3690
#_showast#1 at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:145
_showast at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:143
#_showast#1 at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:149
_showast at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:143
#_showast#1 at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:149
_showast at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:143 [inlined]
show at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:141
unknown function (ip: 0x7fd0b76c1936) at (unknown file)
_jl_invoke at /home/vtjnash/julia1/src/gf.c:3490 [inlined]
ijl_apply_generic at /home/vtjnash/julia1/src/gf.c:3690
_show_default at ./show.jl:511
show_default at ./show.jl:494 [inlined]
show at ./show.jl:489
jfptr_show_70793 at /home/vtjnash/julia1/usr/lib/julia/sys.so (unknown line)
_jl_invoke at /home/vtjnash/julia1/src/gf.c:3490 [inlined]
ijl_apply_generic at /home/vtjnash/julia1/src/gf.c:3690
_show_default at ./show.jl:511
show_default at ./show.jl:494 [inlined]
show at ./show.jl:489 [inlined]
print at ./strings/io.jl:35
print at ./strings/io.jl:46
unknown function (ip: 0x7fd0b76bf44d) at (unknown file)
_jl_invoke at /home/vtjnash/julia1/src/gf.c:3490 [inlined]
ijl_apply_generic at /home/vtjnash/julia1/src/gf.c:3690
#_showast#1 at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:145
_showast at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:143 [inlined]
#_showast#1 at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:149
_showast at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:143 [inlined]
#_showast#1 at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:149
_showast at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:143 [inlined]
show at /home/vtjnash/julia1/doc/deps/packages/MarkdownAST/cMjYL/src/node.jl:141 [inlined]
#domify##18 at /home/vtjnash/.julia/dev/Documenter/src/html/HTMLWriter.jl:2404
unknown function (ip: 0x7fd0b76ba5c2) at (unknown file)
_jl_invoke at /home/vtjnash/julia1/src/gf.c:3490 [inlined]
ijl_apply_generic at /home/vtjnash/julia1/src/gf.c:3690
#sprint#437 at ./strings/io.jl:113
sprint at ./strings/io.jl:106 [inlined]
domify at /home/vtjnash/.julia/dev/Documenter/src/html/HTMLWriter.jl:2404
unknown function (ip: 0x7fd0b76b9d2c) at (unknown file)
_jl_invoke at /home/vtjnash/julia1/src/gf.c:3490 [inlined]
ijl_apply_generic at /home/vtjnash/julia1/src/gf.c:3690
#domify##0 at /home/vtjnash/.julia/dev/Documenter/src/html/HTMLWriter.jl:1707
iterate at ./generator.jl:48 [inlined]
collect_to! at ./array.jl:848
collect_to_with_first! at ./array.jl:826
jfptr_collect_to_with_firstNOT._22026 at /home/vtjnash/julia1/doc/deps/compiled/v1.13/Documenter/sHGjf_DG0H8.so (unknown line)
_jl_invoke at /home/vtjnash/julia1/src/gf.c:3490 [inlined]
ijl_apply_generic at /home/vtjnash/julia1/src/gf.c:3690
collect at ./array.jl:800
map at ./abstractarray.jl:3396 [inlined]
domify at /home/vtjnash/.julia/dev/Documenter/src/html/HTMLWriter.jl:1702 [inlined]
render_article at /home/vtjnash/.julia/dev/Documenter/src/html/HTMLWriter.jl:1496
render_page at /home/vtjnash/.julia/dev/Documenter/src/html/HTMLWriter.jl:901
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.


Close #2505
I have added a new field to HTMLContext
admonition_counterswhich stores counters for each admonition so each one gets a unique identifier. The main challenge was generating unique id for each admonition which is the meat of the pr, added some css for hover functionality of linkBefore :

After :

works well when tested locally