Keep the signature anchor when :noindex: is set - #112
Conversation
HTTPResource.add_target_and_index() appends the per-page anchor unconditionally and only gates the global route registration behind the noindex option, but ObjectDescription.run() skips the whole method when the option is set, so the anchor (and the HTML permalink) is dropped along with the registration and deep links into the page stop working. Anchors are per-document ids and cannot collide across pages; only the global registration can produce duplicate-route warnings (which are order-dependent in parallel builds, where merge_domaindata detects them). Handle the option in run() instead: pop it so the base class calls add_target_and_index(), keep the anchor, and skip only the registration. The informational noindex attribute on the desc node is preserved. Tested against Sphinx 6.2.1, 7.0.1, 7.4.7 and 8.1.3.
Documentation build overview
|
stevepiercy
left a comment
There was a problem hiding this comment.
Thanks for reporting the issue and creating a PR. Sorry for the long delay to respond.
Do you have before and after examples as rendered HTML? It's not necessary, but it would help me have a complete understanding. Our PR preview docs on Read the Docs don't show any relevant diff.
Otherwise, this LGTM. I have a few PRs ahead of this to merge, then I'll handle any merge conflicts with CHANGES.rst.
|
Thanks Steve! Here's a concrete before/after from the real case this came from. Salt's Rendered Before (2.0.0) -- no <dt class="sig sig-object http"><span ...>POST</span><span ...>/</span></dt>After (this PR): <dt class="sig sig-object http" id="post--"><span ...>POST</span><span ...>/</span><a class="headerlink" href="#post--">...</a></dt>
Why the RTD preview shows no diff: httpdomain's own docs never document a route on two pages, so nothing there is |
Fixes #111.
add_target_and_index()already expresses the intended split - the per-page anchor is appended unconditionally and only the global route registration is gated behind:noindex:- butObjectDescription.run()skips the whole method when the option is set (at least since Sphinx 2.4.5), so the anchor and the HTML permalink were dropped along with the registration.Handle the option in
HTTPResource.run()instead: record it, pop it before running the base class (soadd_target_and_index()is called), keep the anchor, and gate only the registration. The informationalnoindex/no-indexattributes on thedescnode are preserved. On Sphinx 7.2+ the pop also happens before the base class normalizes the legacy spelling intono-index, so behaviour is identical across the supported range.Anchors are per-document ids, so restoring them cannot introduce collisions; only the global registration participates in
merge_domaindata(), which is where duplicate routes are (order-dependently) detected in parallel builds.Includes a build-level test covering both halves: the
:noindex:'d copy of a route documented on another page keeps itspost--demoanchor, and the domain data contains only the canonical page's registration. The anchor assertion fails without the fix; the registration assertion passes with and without, matching the analysis.Validated with
pytest(full suite) andsphinx-build -W -b html docson Sphinx 8.1.3 (the locked dev version), and the new tests additionally against Sphinx 6.2.1, 7.0.1, and 7.4.7.Real-world motivation: saltstack/salt documents overlapping routes on three pages and hit nondeterministic
duplicate HTTP post method definitionfailures in-W -j autoCI builds (saltstack/salt#69724);:noindex:on the non-canonical copies fixes that but currently costs the anchors, which this change makes unnecessary to trade away.