Skip to content
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

Handle explicit intersphinx inventory names in all domains #236

Merged
merged 3 commits into from
Oct 25, 2022

Conversation

shiftinv
Copy link
Contributor

@shiftinv shiftinv commented Oct 5, 2022

Intersphinx supports explicitly specifying the inventory name in a link target, e.g. the python: part in :ref:`python:datetime-datetime` .

The custom missing_reference hook in hoverxref handles such target names as well, but only in the std domain (as far as I can tell, particularly for :ref:, which is in the std domain).

Intersphinx supports target names with explicit inventories regardless of role/domain, which means :py:attr:`python:datetime.time.minute` results in the correct link, but without the hoverxref popup as the python: prefix isn't handled by hoverxref in this case.


This PR attempts to fix this by removing the check for the std domain, which I believe intersphinx also doesn't check.

I'm very much not sure if this makes sense with how Sphinx, Intersphinx and sphinx-hoverxref interact, or if there are side-effects, I don't know a whole lot about Sphinx internals 😅 Nevertheless, this fixes the aforementioned issue, and tests pass fine :)

The difference between sphinx-hoverxref 1.2.0 and this PR can be seen here:

Note the difference on the second :attr: link, while hovering over the second :ref: link works in both versions.


📚 Documentation preview 📚: https://sphinx-hoverxref--236.org.readthedocs.build/en/236/


📚 Documentation preview 📚: https://read-the-docs-sphinx-hoverxref--236.com.readthedocs.build/en/236/

@onerandomusername
Copy link

Would you mind updating this to be based on version 1.2.0?

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow @shiftinv! I'm pressed of the quality of this PR. The description is awesome, the live examples and the code! Thanks a lot for working on this 🤗

I don't remember exactly why I made the decision to only check for std domain. I suppose it was a way to protect myself while coding this because I don't have a strong knowledge of Sphinx internals either. However, I think it's find and seems safe to accept this change since Sphinx itself is doing something pretty similar.

I will release 1.3.0 soon.

@humitos humitos self-assigned this Oct 25, 2022
@humitos humitos merged commit ad8a82d into readthedocs:main Oct 25, 2022
@humitos
Copy link
Member

humitos commented Oct 25, 2022

@shiftinv @onerandomusername I just released 1.3.0. Please, let me know if it works as you expected 💯

@shiftinv
Copy link
Contributor Author

@humitos Awesome, much appreciated! Everything seems to be working fine, all references are now being found 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants