[rst] Improve unreferenced footnote warnings #12730
Conversation
sphinx/transforms/__init__.py
Outdated
| @@ -294,23 +295,40 @@ class UnreferencedFootnotesDetector(SphinxTransform): | |||
| Detect unreferenced footnotes and emit warnings | |||
| """ | |||
|
|
|||
| default_priority = 200 | |||
| default_priority = Footnotes.default_priority + 1 | |||
There was a problem hiding this comment.
Could this change of priority be breaking extensions that would have relied on the absolute priority?
I'm worried that some extensions could have fixed their internal footnotes by adding a priority UnreferencedFootnotesDetector.default_priority - 1 which would now hit the same priority as the footnotes transformation.
There was a problem hiding this comment.
Well, I think there would be a very slim chance of that 😅, and obviously this transform doesn't actually make any changes to the doctree so it won't have any indirect effect on other transforms.
If you want, I can change the existing transform to a "no-op", then add this as an additional new transform?
It's of note, that I'm doing this as a result of executablebooks/MyST-Parser#931; so that I can remove ad-hoc fixes in myst-parser and just let sphinx handle warnings
There was a problem hiding this comment.
No let's not overcomplicate the things (i.e. keep what you've written, it's ok). But add a CHANGELOG entry for that by saying that the priority has been changed (I don't know if we have it, but a table with the transformations and post-transformations priorities would be nice...).
There was a problem hiding this comment.
I don't know if we have it, but a table with the transformations and post-transformations priorities would be nice...
Nope I don't believe so, it's on the TODO list 😄
|
@chrisjsewell quickly on commit style -- please avoid '[blah]' in favour of either unprefixed or prefix with a component (e.g. 'intersphinx:'). I didn't get a chance to review this before merge, but will have a look at some pont. A |
yep no problem, perhaps we could add a little more to https://www.sphinx-doc.org/en/master/internals/contributing.html#contribute-code to suggest best practices for such things. |
Footnotes are now parsed similar to the corresponding restructuredtext, in that resolution (between definitions and references) and ordering is now deferred to transforms on the doctree. This allows for the proper interaction with other docutils/sphinx transforms, including those that perform translations. In addition, an upstream improvement to unreferenced footnote definitions is also added here: sphinx-doc/sphinx#12730, so that unreferenced and duplicate definitions are correctly warned about, e.g.: ``` source/index.md:1: WARNING: Footnote [1] is not referenced. [ref.footnote] source/index.md:4: WARNING: Duplicate footnote definition found for label: 'a' [ref.footnote] ``` It is of note that warnings for references with no corresponding definitions are deferred to docutils to handle, e.g. for `[^a]` with no definition: ``` source/index.md:1: ERROR: Too many autonumbered footnote references: only 0 corresponding footnotes available. [docutils] source/index.md:1: ERROR: Unknown target name: "a". [docutils] ``` These warning messages are a little obscure, and it would be ideal that one clear warning was emitted for the issue. However, it is non-trivial in this extension; to both suppress the existing warnings, and then replace them with a better one, so for now we don't do it here, and ideally this would be improved upstream in docutils.
The current
UnreferencedFootnotesDetectortransform (added in 01cb3a7) is untested and misses warnings for a number of cases.In this PR I change how the detection works, by moving the transform to run just after the docutils
Footnoteresolution transform; https://github.com/live-clones/docutils/blob/266f359408130ca25f6f161453140e73f169408a/docutils/docutils/transforms/references.py#L410,then simply check for any footnotes without "back-references".
I believe this is simpler to reason about and is no longer trying to pre-empt the resolution.
In the newly added test for this transform, if you compare to the previous transform, you will see that it now picks up previously missed unreferenced footnote (for symbols and labelled auto-numbered footnotes)
edit: the failures against
Docutils HEADappear unrelated to this PR