docs: give each best-practice admonition a stable :name: anchor#2524
docs: give each best-practice admonition a stable :name: anchor#2524tonyandrewmeyer wants to merge 10 commits into
Conversation
Add a :name: option to every Best practice admonition in the operator
howto docs, so each practice has its own anchor in the rendered HTML
page (e.g. manage-libraries.html#best-practice-libraries-no-status-mutation)
and a stable identifier independent of its surrounding text.
Update .github/update-best-practice-table.py to extract :name: from each
admonition and emit two things in the generated docs/reuse/best-practices.txt:
- a Sphinx anchor `(<name>)=` before each bullet, so the bullets on
the collation page (follow-best-practices section) are also
individually deep-linkable
- a trailing `<!-- id: <name> -->` HTML comment, so downstream
automation (e.g. canonical/charmhub-listing-review) can match
practices by ID rather than fuzzy-matching the description text
A header comment at the top of best-practices.txt documents the convention.
This is the operator-side of the change; a matching PR against charmcraft
will add :name: to the Best practice admonitions there. Admonitions
without :name: continue to render exactly as before.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The generated best-practices.txt emits a Sphinx anchor ``(<name>)=`` before
each bullet so the collated list is individually deep-linkable. When the
admonition lives on the same page that ``{include}``s the collation, that
anchor collides with the admonition's own ``:name:`` target and Sphinx
fails the build with a duplicate-target warning under --fail-on-warning.
Detect include hosts at generation time and skip ``(<name>)=`` for their
admonitions; the admonition's ``:name:`` already provides the anchor on
that page, and the trailing ``<!-- id: <name> -->`` comment is unchanged
so downstream automation is unaffected.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The header at the top of the generated best-practices.txt is an HTML comment, but it contained an example of the trailing per-bullet id comment that included a literal ``-->`` sequence. HTML comments terminate at the first ``-->``, so the outer comment closed early and the remainder of the header rendered as visible text in the ``Follow best practices`` section of the host page. Reword the header so it describes the syntax without including a literal closing sequence, and add a Python-level note explaining why. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
dwilding
left a comment
dwilding
left a comment
There was a problem hiding this comment.
Do you have a plan to make use of the ability to link to the bullets under "Follow best practices"? The anchors aren't discoverable when looking at the page in a browser, so unless you have anything planned for the review process, I think we should drop those anchors.
I think the ability to link directly to the best practice notes is more important. But this PR doesn't implement that for the "See foo" links at the end of each bullet. I'd be happy to review that in the same PR, if you want to implement it.
Ah, I thought that they would be there in the HTML, so usable even if not easily. But it seems not.
I'll see if this is simple enough to do in this PR. |
Each best-practice admonition already gets a stable id from its :name:, but the anchor is invisible in the rendered page. Inject a ¶ headerlink into the admonition title (revealed on hover) so readers can copy a deep link the same way they would from a heading. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@dwilding i added a wee anchor via JS, what do you think? |
|
Sorry for the delay in getting back to this one. I like the little link anchors on the best practice notes! Unless I'm missing something, I still think it would be worth removing the targets from - (best-practice-no-duplicate-model-config)=
- Don't duplicate model-level configuration options that are controlled by {external+juju:ref}`juju model-config <command-juju-model-config>`. See [Define a configuration option](#define-a-configuration-option). <!-- id: best-practice-no-duplicate-model-config -->I don't see a strong need to be able to deep link to the bullets. |
Per review, deep links to the bullets on the collation page aren't needed: the JS hover anchors on the source admonitions cover linking to individual practices, and the trailing <!-- id: ... --> comments cover downstream automation. Stop emitting the (slug)= targets, and remove the include-host detection that only existed to avoid duplicating them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dwilding
left a comment
dwilding
left a comment
There was a problem hiding this comment.
Will you do a separate PR to update the links at the end of each bullet so that they point to the :name: anchors?
Per review, point the trailing "See" link on each collated bullet at the practice's own :name: anchor, so readers land on the exact admonition rather than the enclosing section. Also give bullets that previously had no "See" link (practices that aren't under a section heading) a link, using the page title as the link text. The charmcraft-sourced bullets are unchanged; those admonitions don't have :name: options yet and will be handled in a separate charmcraft PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
I was going to but it turns out it's only a few lines of code so I've done it now. What do you think? It's nice they go straight there and it adds links to a couple that were missing. But you do jump to a place that is maybe after the context, since the best practice is often after the explanation. I'm not sure this is better than linking to the section containing the best practice. |
|
There's also the accessibility angle of having four (!) identically-named links that go to different places: I think we revert this change. It was nice in principle, but I don't think it's better in practice. I'd say keep the visible anchors you added to the best practice notes though! |
Landing on the enclosing section gives the reader more context than landing on the admonition itself, and retargeting also produced multiple links with the same text but different destinations (e.g. the three logging practices all linked as "How to log from your charm"). Keep the :name:-anchor fallback only for practices that previously had no "See" link at all, and only when the link text appears once on the page, so identically-named links can't point at different destinations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
This PR adds a
:name:option to every best practice admonition in the docs, so each practice has its own anchor in the rendered HTML page (e.g. manage-libraries.html#best-practice-libraries-no-status-mutation) and a stable identifier independent of its surrounding text. A little JS makes the anchor visible so that people can copy it.There are two benefits here:
The
.github/update-best-practice-table.pyscript is updated to extract:name:from each admonition and emit, in the generated docs/reuse/best-practices.txt, a trailing<!-- id: <name> -->HTML comment, so downstream automation (particularly canonical/charmhub-listing-review) can match practices by ID rather than fuzzy-matching the description textA header comment at the top of best-practices.txt documents the convention.
A matching PR against charmcraft will add :name: to the Best practice admonitions there (assuming this lands). Admonitions without :name: continue to render exactly as before.
Preview