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

rst: rst2html and doc now have clickable anchors for each paragraph/list element #17251

Closed
wants to merge 29 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Mar 4, 2021

/cc @a-mr since you're our RST expert ;-)

fixes most of nim-lang/RFCs#117, plus a few other related improvements.
We can now point to any paragraph/list element in docs (rst or nim) and use it to share links. Ones in between stable anchor names (derived from a title or an anchor name) are considered non-stable and start with - to denote this. They're unstable in the sense that if a new paragraph/list element is added in the interval anchoredName .. generatedAnchoredName, the numbering may be off. This is expected and allows working with the common case of paragraphs/list elements that don't have an anchor name, yet still allow sharing links.

there are 3 kinds of anchors:

  • ones from title (eg Best practices below): anchor name is derived from title
  • ones from explicit anchor (eg .. _noimplicitbool below): anchor name is explicitly provided and shouldn't change. Likewise for [display](link) markdown syntax
  • ones in between (eg foo, bar): anchor name is derived from the last seen anchor name, plus an incrementing counter.
Best practices
==============

.. _noimplicitbool:
Take advantage of no implicit bool conversion

1. foo
2. bar

design notes

  • I'm ignoring hierarchy when deriving intermediate anchor names, it's the simplest and produces good results, try it, eg: nim rst2html -r doc/contributing.rst or nim rst2html -r --doccmd:skip doc/manual.rst

TODO before merging

  • CI failures
    only tests/stdlib/trstgen.nim fails, I'll fix it once the formatting in this PR is accepted to avoid back and forth on editing the tests

  • $nimb r nimdoc/rsttester.nim fails, I'll fix it once the formatting in this PR is accepted to avoid back and forth on editing the tests

  • see how this shows anchors on mouse over (hover) https://sublime-and-sphinx-guide.readthedocs.io/en/latest/references.html (eg when hovering on right of Links to External Web Pages)

future work

  • (pre-existing) subsection titles shouldn't depend on parent name, it's a false good idea IMO; if parent name changes, it invalidates more links. The rare case of duplicates titles should be handled instead differently (eg error/warning):
    https://nim-lang.github.io/Nim/manual.html#distinct-type-avoiding-sql-injection-attacks
    =>
    https://nim-lang.github.io/Nim/manual.html#avoiding-sql-injection-attacks
  • (pre-existing) rstnodeToRefnameAux shouldn't translate - as minus, this is anchor name is bad:
    https://nim-lang.github.io/Nim/manual.html#types-preminusdefined-integer-types
  • generate anchors as well for code blocks, runnableExamples, and infer anchor name from symbols in nim doc in above logic
  • address simpler doc links: getFilePermissions <#getFilePermissions,string>_ => $getFilePermissions RFCs#125

@timotheecour timotheecour marked this pull request as ready for review March 4, 2021 08:49
var idLast = 0
proc impl(n: PRstNode) =
if n == nil: return
n.anchorGen = "\0" # special char to indicate we've visited this node.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? It's supposed to be a strict tree structure or at least an acyclic graph.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's needed because I'm processing the whole tree/DAG upon 1st call to renderRstToOut, and because renderRstToOut is reentrant. This sentinel avoids needs for a separate flag.
Without this logic, you can see it doesn't work:
nim rst2html -r doc/contributing.rst
the anchor next to separate test files would be #-ROOT-2 instead of #-writing-tests-3

@a-mr
Copy link
Contributor

a-mr commented Mar 4, 2021

The idea is great.

The current style is too visually disturbing.

I'd like to put these marks outline: either to the left side margin of the text (where yellow dots are on the picture) or to the right margin (green dots):

image

Unfortunately I don't know how to do it in HTML. But I think I've seen something like that somewhere.

@@ -97,6 +97,7 @@ type
anchor*: string ## anchor, internal link target
## (aka HTML id tag, aka Latex label/hypertarget)
sons*: RstNodeSeq ## the node's sons
anchorGen*: string ## generated anchors from context
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this additional field really needed? When anchor is already defined you just use it; when it's not — you generate it. Anchor is always unique for any node.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed anchorGen (and reuse anchor), but added anchorKind which determines the kind of anchor (needed for book-keeping, among other things to make this work:

if n.anchorKind == raDefault: computeAnchorGen(n)

and knowing the kind of the anchor can be useful for other purposes.

@stale
Copy link

stale bot commented Mar 22, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Mar 22, 2022
@stale stale bot closed this Apr 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants