-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Intersphinx, refactoring and intersphinx_disable_domains #9459
Intersphinx, refactoring and intersphinx_disable_domains #9459
Conversation
@adamtheturtle, @nijel, and @phlax, does this solve your original issues? |
@jakobandersen I'm no longer on the project with the issue so I can't really say. |
i can test |
It seems that it should address the issue I had. Unfortunately, I probably won't have time to test this in next two weeks... |
I just tested The name
But I can't think of a better name that is not super long ( Some more comments on your (super thorough!) pull request overview:
I'm not sure I follow. The |
(Typo fixed)
Right, I got to the same conclusion. I was thinking of trying to get "implicit" in there, but did find a good name.
Hmm, like the standard domain, I guess they contain roles/directives that are not naturally described in terms of a domain. I don't know the details of them, and haven't used them my self, but some bits I could find:
Yes, it looks like the math domain seems to contain equation references already for internal resolution, so I don't immediately see why they couldn't be exposed via intersphinx. I guess no user has requested it before?
For references generated from roles intended to reference a single entity, yes, there the Python domain (and perhaps js and rst?) support the prefix to some degree. However, this is because they either don't need to perform detailed parsing of the role argument, or they specifically need to take intersphinx into account. This is why the prefix in a C/C++ role gives a parsing warning/error (#8418). .. py:function:: f(i: int) -> dict Here |
Thanks a lot for the clarifications about these extra roles and directives. I am still trying to make sense of the Sphinx API and how it is described in the documentation.
Done :) #9483
Thanks! That's what I understood from your previous comments. I was distracted by your wording here, I now get that programming-language domains don't necessarily or always support explicit inventory specification (aka intersphinx prefixes). FWIW, this looks like a good addition to me and I'm looking forward for this to pave the way towards #9062. |
doc/usage/extensions/intersphinx.rst
Outdated
|
||
For example, with ``intersphinx_disabled_domains = ['std']`` a cross-reference | ||
``:doc:`installation``` will not be attempted to be resolved by intersphinx, but | ||
``:doc:`otherbook:installation``` will be attempted to be resolved in the |
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 think explicit references should be also disabled. It would be replaced by #9062 (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.
Note that I wrote this PR as if #9062 (comment) didn't exist. My plan is to have the functionality change incrementally:
- This PR, for which this part is as intended: the user must intentionally write something intersphinx-specific to activate it for the disabled domains.
- Then an updated Add intersphinx role #9062 which uses the functionality in this PR. By then the old style of explicit intersphinx references, as the one you quoted here, can be deprecated/removed.
- The whole domain delegation part.
So in short, yes, once an updated #9062 has been merged, I don't mind this option disabling the explicit ones as well, but I don't think it is harmful if they are not.
@jakobandersen i tested this branch this morning (here envoyproxy/envoy#18090) and afaict everything is working as i would hope with non/intersphinx ref links - thanks for addressing this lmk if you would like any further testing |
7bc3682
to
cbbd8c9
Compare
@phlax, thanks for the testing! |
@tk0miya, rebased and quickstart stuff removed. I think this is good to merge, assuming something like the following incremental roadmap:
|
Note: I found #9626 contains remote |
59440aa
to
fe6f2d0
Compare
Doh, right. Disabling all of
The breaking change to the master branch after this PR is then to default this option to |
Anything we can do to move this forward? It would be great to have this in Sphinx 4.3, I could play with it a bit and document it as part of the tutorial. |
32c5467
to
df78eb2
Compare
honor_disabled_refs: bool, | ||
node: pending_xref, contnode: TextElement) -> Optional[Element]: | ||
# disabling should only be done if no inventory is given | ||
honor_disabled_refs = honor_disabled_refs and inv_name is None |
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.
Is there any case that honor_disabled_refs
is True, but inv_name
is None?
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.
Yes, that is the default for most pending_xref
:
- From the
missing_reference
callback it will honor the disabling: https://github.com/jakobandersen/sphinx/blob/778a3fe6a7a4caf9152caa5c9b10d5378de671c2/sphinx/ext/intersphinx.py#L467 - going through
resolve_reference_detect_inventory
: https://github.com/jakobandersen/sphinx/blob/778a3fe6a7a4caf9152caa5c9b10d5378de671c2/sphinx/ext/intersphinx.py#L446 - and then in
resolve_reference_any_inventory
theNone
is added: https://github.com/jakobandersen/sphinx/blob/778a3fe6a7a4caf9152caa5c9b10d5378de671c2/sphinx/ext/intersphinx.py#L428
LGTM with nits. |
Also, when a reference is unresolved, don't strip the inventory prefix.
Fixes sphinx-doc#2068 Replaces sphinx-doc#8981
df78eb2
to
0d9f4cd
Compare
And change format for domains to {name}:*
Great, thanks! All nits addressed basically as suggested, so I'll merge once CI is green. |
See discussion in sphinx-doc#9459
🎉 |
Feature or Bugfix
Purpose
In short, this PR implements #8981 (comment) and thus fixes #2068 and replaces #8981 (second commit) and part of #8929 (first commit).
At the same time the PR serves as a basis for a better implementation of #9062, and contains part of the refactoring in #8929.
Detail
The first commit is primarily refactoring, though in the case where a target contains a prefix
something:
and the resolution fails, the old code would strip the prefix in the final output. Now this prefix is shown in the output to avoid hiding the information from the source code. I consider this a minor "bug" fix.The refactoring consists splitting the resolution into multiple functions:
resolve_reference_in_inventory
: lookup in a user-specified inventoryresolve_reference_any_inventory
: lookup in any inventoryresolve_reference_detect_inventory
: try firstany
and otherwise try to extract a prefix from the target. Themissing_reference
handler directly calls this one._resolve_reference_in_domain
and_resolve_reference_in_domain_by_target
. Abstractly, these are the ones that need to be split and moved to the domain classes (see Intersphinx delegation to domains #8929).The second commit implements
intersphinx_disable_domains
which defaults to[]
for backwards compatibility. For now I addedto the
conf.py
generated bysphinx-quickstart
. The rationale is that I believe:ref
and:doc:
references most often are intended to be local, and a simple typo or missing file may silently make them resolve to external documents (i.e., the problem discussed in #2068 and #8981. All other built-in domains either do not export objects for intersphinx (changeset
,citation
,index
,math
) or they are programming language domains (c
,cpp
,js
,py
,rst
) which generates cross-references in declarations where the user can not add an inventory specification.Note,
std
also contains the object typesterm
,token
,envvar
, andcmdoption
, but my guess is that users rarely have external links for these, and in all cases one can provide an explicit inventory specification.