-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
rST: add notice about duplicate hyperlink target names #13400
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
base: master
Are you sure you want to change the base?
rST: add notice about duplicate hyperlink target names #13400
Conversation
|
The changes in 4df6239 seem to introduce a large number of false-positive warnings; numbered footnotes, for example, are frequently a source of distraction with this in place. I'll move this pull request back into draft status until I can resolve that problem. |
…tions Re-uses some existing filtering for footnotes and other acceptable duplicates. Some false-positives continue to occur with this change in place; for example, internal hyperlink targets that are subsequently `include`d into other docs.
|
Todo: extend the test coverage to add an example of document that is |
| .. _ambiguous_shared_hyperlink: | ||
|
|
||
| This hyperlink target is declared once and is included by multiple documents. |
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.
@AA-Turner @picnixz is there a way to determine a unique declaration identifier/path -- e.g. where a hyperlink was defined, instead of where it is used?
When reporting warnings about duplicated anonymous hyperlinks, it'd be nice to be able to indicate clearly that the duplication originates from multiple includes of a common document, instead of weird/confusing warnings about the hyperlinks appearing in multiple documents.
|
The term "anonymous" in the name of this PR is confusing:
Anonymous hyperlink targets don't use a reference name and may be internal or external. Example: In every document, the number of anonymous references must match the number of anonymous targets. |
|
Thank you @gmilde - is edited version (rST: add warning about duplicate-declaration hyperlink targets) reasonably accurate? |
|
I'd call it "rST: add warning about duplicate hyperlink target names". |
|
Thank you; updated. |
|
Regarding spurious warnings, see also my comment on the issue page. |
|
@AA-Turner thanks for adding the 9.0.0 milestone - I worry about the potential for this to cause build breakage for potentially lots of downstream Sphinx projects, but I would also like for it to be included in an upcoming release. |
AA-Turner
left a comment
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.
Can we use an INFO level notice for now, and add a note to upgrade it to WARNING in eg Sphinx 11?
A
|
Seems reasonable, yep. Perhaps I'll mention it on the release thread too for inclusion, just in case anyone feels like testing it (or at least being aware of it) in advance. |
…arget-warning Conflicts: CHANGES.rst
|
|
||
| ENV_WARNINGS = """\ | ||
| {root}/autodoc_fodder.py:docstring of autodoc_fodder.MarkupError:\\d+: \ | ||
| DUPLICATE_LABEL_WARNINGS = False |
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.
@jayaddison perhaps just remove this? We can add it if/when the message is upgraded to a warning.
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.
@AA-Turner debatable, I admit - but I opted to prefer projecting the test coverage unchanged into the future, since the output seems reasonably unlikely to differ, and the rework involved could be an annoyance for someone (likely myself). It'd also provide a little more reassurance to someone inspecting a diff (again, sometimes myself in other contexts) that the author did in fact at least add test coverage.
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.
As a first preference instead of removing it entirely, I think I'd prefer to figure out whether there's a more maintainer-friendly way to express it (e.g. some mild misuse of deprecation patterns, or similar).
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.
@jayaddison happy for you to have a look, but otherwise I'd prefer to remove it as dead code.
A
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.
My previous comments were word salad; I think what I should have explained up-front is: I do want this to become a warning, and I am fairly adamant about that.
So I would like to write the tests in a way that prepares for warning-level messages, while permitting info-level until then.
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'm still on the fence about making this a warning. I opened #14187 as an idea that would give me more certainty, but that's a large project.
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.
My one and only Linux kernel patch (so far, at least) disambiguated two duplicate labels, and that allowed the documentation to build reproducibly. If at all possible, I'd prefer for that not to regress (I don't anticipate that that exact label would be re-duplicated, but the same category of label-duplication problem could easily occur again elsewhere in the documentation).
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 do also agree with providing advance notice and a gradual rollout though, given uncertainty about the scale of the problem)
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.
Actually, I lie - my patch was one of a few fixes that has improved the reproducibility of the Linux kernel docs, but there is at least one other remaining nondeterminism issue in Sphinx that is relevant there.
| if version_info >= (11, 0, 0): | ||
| # this code can be removed when we prepare Sphinx 11 for release | ||
| msg = 'From Sphinx 11, duplicate labels should always warn by default' | ||
| raise RemovedInSphinx11Warning(msg) | ||
| else: | ||
| DUPLICATE_LABEL_WARNINGS = version_info >= (10, 0, 0) |
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 isn't the right approach: we shouldn't raise warnings as exceptions, nor does it make sense to raise a module-level warning in a test file.
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.
It doesn't make sense, but it would only occur when we begin developing Sphinx 11 -- the major release after the info-level messages are upgraded to warnings. At that point, the test condition could be removed.
What's a better way to annotate test code that can be cleaned up after a certain version?
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.
What's a better way to annotate test code that can be cleaned up after a certain version?
# xref RemovedInSphinxXXWarning
Whoever does the removals needs to search for the relevant name, so we often put things in comments.
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.
Ok, perfect. I'll adjust the test code to follow that pattern. Thank you!
|
I'm going to delay this to 9.1. A |
…arget-warning Conflicts: CHANGES.rst
|
@AA-Turner this is probably too late for the 9.1.0 boat (I notice the rc1 ship has sailed), but should be ready for review. |
Purpose
The goal of this pull request is to identify duplicate internal hyperlink targets -- the presence of these can cause documentation hyperlinks to resolve ambiguously (e.g. sometimes incorrectly) at build-time.
References