Intersphinx - RefType - Use objtypes inventory as fallback mechanism #144

chrizzFTD · 2021-09-19T09:52:38Z

Hi, thanks a lot for this awesome sphinx extension!

Issue

I came across a small issue where intersphinx class methods would not load hoverxref tooltips on my project at grill.rtfd.io. This was strange since classes from the same intersphinx project would work fine.

Discoveries

On my RST doc file, I refer to the intersphinx grill.names.UsdAsset.get_anonymous method like so:

 :py:meth:`grill.names.UsdAsset.get_anonymous`

For hoverxref, the inventory loaded here:

sphinx-hoverxref/hoverxref/extension.py

Lines 225 to 226 in eed8700

    
           for inventory_name in app.config.hoverxref_intersphinx: 
        
               inventory = inventories.named_inventory.get(inventory_name, {})

Looks like this:

'py:method': {'grill.names.DateTimeFile.get_pattern_list': ('grill.names',
                                                             '2.5',
                                                             'https://grill-names.readthedocs.io/en/latest/names/DateTimeFile.html#grill.names.DateTimeFile.get_pattern_list',
                                                             '-'),
               'grill.names.UsdAsset.get_anonymous': ('grill.names',
                                                      '2.5',
                                                      'https://grill-names.readthedocs.io/en/latest/names/UsdAsset.html#grill.names.UsdAsset.get_anonymous',
                                                      '-')},

If I replace the RST text to use method like so:

 :py:method:`grill.names.UsdAsset.get_anonymous`

The translation fails with a message like:

WARNING: Unknown interpreted text role "py:method".

So it looks like it must be left as py:meth.

Proposal

I've chosen to use a "fallback" mechanism as presented on this PR.

You can see it working on the hint block from this docs:

If this PR is not needed and there is something different I should do on my side, please let me know!

astrojuanlu · 2021-09-19T12:14:58Z

cc @humitos

humitos

Hi! Thanks for your PR! 🤟🏼

I think this is correct. I haven't tested it yet, tho.

It would be good to have some tests for this case.

Also, are you aware of other synonyms (like py:method and py:meth) that may make sense to add here as well? In that case, we should add them as well.

Also also, do you know if this works for all domains or only for Python (e.g. py:)? In that case, we should restrict it to Python only.

chrizzFTD · 2021-09-22T10:49:52Z

Thanks, @humitos! I'll look into adding some tests for this.

About the other questions, I'm not aware about other synonyms or the python only domain, I'll update the code to handle only python in the meantime :P

humitos · 2021-09-23T16:36:01Z

I found another case. :py:mod: is what Sphinx expects, but "intersphinx inventory" from CPython is defined as :py:module:.

Borrowed from #144

humitos · 2021-09-28T10:55:20Z

@chrizzFTD Hi! I included your solution in #146 because I found myself having this issue. However, I didn't finish this work.

It would be good if you have some time to update your PR with the latest changes from master (the code was simplified a lot) and add tests so we can merge this PR and be covered 😄

# Conflicts: # hoverxref/extension.py

chrizzFTD

Hi @humitos, I've updated my branch to take the recent changes from master. I've also taken a new approach by letting fallback keys to be given by the domain for that inventory + tests.

Please let me know what you think!

One thing to note, however, is that it looks like something else might have broken recently as my project grill is no longer able to display tooltips when using this branch. Everything, both internal and intersphinx references now get stuck in a "Loading" tooltip:

If I check on an older build from a couple of weeks ago, e.g. on https://grill.readthedocs.io/en/develop/api.html, the problem does not appear. Do you know what might be the issue?

chrizzFTD · 2021-10-02T12:50:33Z

tests/examples/intersphinx/index.rst

@@ -4,9 +4,13 @@ Example page
 This is an example page.

 :ref:`This a :ref: to The Python Tutorial using intersphinx <python:tutorial-index>`.
-:ref:`This a :ref: to datetime.datetime Python's function using intersphinx <python:datetime-datetime>`.


I chose to remove these single quotes since they were causing trouble on my end, they came as:

Pythonâ€™s

Found it easier to remove them since I don't find them relevant to this test, but please let me know if I should bring them back.

This seems to be an encoding problem on your side. You may not be declaring the encoding properly.

chrizzFTD · 2021-10-02T12:51:32Z

tests/examples/intersphinx/index.rst

+
+:py:meth:`collections.Counter.elements` to test intersphinx methods.
+
+:py:func:`sum` to test intersphinx functions.


Without the changes, these two lines above do not have the "hoverxref ... reference external" html class added.

humitos · 2021-10-04T14:55:45Z

If I check on an older build from a couple of weeks ago, e.g. on grill.readthedocs.io/en/develop/api.html, the problem does not appear. Do you know what might be the issue?

Yes. It's already fixed. There was a problem with CORS and the fix didn't go out yet. I solved it at readthedocs/readthedocs.org#8543. We are deploying that fix tomorrow and the tooltips should work properly in your docs with this branch.

humitos · 2021-10-06T11:12:17Z

@chrizzFTD can you confirm that tooltips work on your documentation using master branch now?

humitos

Looks really good! The new approach is way better since it will always bring all the supported reftypes. I left some suggestions to consider. Let me know!

humitos · 2021-10-06T11:17:09Z

tests/test_htmltag.py

        # Read the Docs' link does have hoverxref enabled
-        r'<a class="hoverxref modal reference external" href="https://docs.readthedocs.io/en/stable/config-file/v2.html#python" title="\(in Read the Docs v\d\d?.\d\d?.\d+\)"><span class="xref std std-ref">This a :ref: to Config File v2 Read the Docs’ page using intersphinx</span></a>',
+        r'<a class=\"hoverxref modal reference external\" href=\"https://docs.readthedocs.io/en/stable/config-file/v2.html#python\" title=\"\(in Read the Docs v\d\d?.\d\d?.\d+\)\"><span class=\"xref std std-ref\">This a :ref: to Config File v2 Read the Docs page using intersphinx</span></a>',


These \" are not required. It should just work with regular " since the string is defined with '

This double quote character was not being escaped by the regular expression when I ran the tests locally, so it never matched, do you know of any setting that would cause this?

humitos · 2021-10-06T11:19:15Z

tests/test_htmltag.py

+        r'<a class=\"hoverxref tooltip reference external\" href=\"https://docs.python.org/3/tutorial/index.html#tutorial-index\" title=\"\(in Python v3.\d\d?\)\"><span class=\"xref std std-ref\">This a :ref: to The Python Tutorial using intersphinx</span></a>',
+        r'<a class=\"hoverxref tooltip reference external\" href=\"https://docs.python.org/3/library/datetime.html#datetime-datetime\" title=\"\(in Python v3.\d\d?\)\"><span class=\"xref std std-ref\">This a :ref: to datetime.datetime Python function using intersphinx</span></a>',
+        r'<a class=\"hoverxref modal reference external\" href=\"https://docs.python.org/3/library/functions.html#float\" title=\"\(in Python v3.\d\d?\)\"><code class=\"xref py py-class docutils literal notranslate\"><span class=\"pre\">float</span></code></a>',
+        r'<a class=\"hoverxref tooltip reference external\" href=\"https://docs.python.org/3/library/collections.html#collections.Counter.elements\" title=\"\(in Python v3.\d\d?\)\"><code class=\"xref py py-meth docutils literal notranslate\"><span class=\"pre\">collections.Counter.elements\(\)</span></code></a> to test intersphinx methods.</p>',


Can you add a comment here that this case is particular and it's for py:meth (fallbacks) ? Also, if it's even clear to explain/understand, we can create a specific test case for these

humitos · 2021-10-06T11:25:41Z

hoverxref/extension.py

+        # Intersphinx inventories may come with concrete types for domain roles.
+        # (e.g. for python, when reftype=='meth', inventory uses "method" or "classmethod"...
+        # so check in order, starting from the current reftype.
+        keys_to_check = [reftype, *env_domain.objtypes_for_role(reftype)]


Suggested change

keys_to_check = [reftype, *env_domain.objtypes_for_role(reftype)]

keys_to_check = set([reftype, *env_domain.objtypes_for_role(reftype)])

In some cases reftype is already included in the response from env_domain.objtypes_for_role(reftype), so we can create a set() to remove duplicated ones.

chrizzFTD · 2021-10-11T15:04:00Z

Thanks @humitos ! I'll come back to this hopefully next weekend

humitos · 2022-05-30T12:56:20Z

I think #171 should solve this issue as well. @chrizzFTD can you please confirm that the latest commit from main branch does work as you expected?

humitos · 2022-06-14T12:52:24Z

@chrizzFTD We released a new version (v1.1.1) with the PR linked on it. I suppose this problem is solved now. However, feel free to re-open this PR or comment here if you find something wrong. Let me know!

chrizzFTD · 2022-09-04T13:58:20Z

Hi @humitos, thanks a lot for the updates & apologies it took me so long to come back here on GitHub; but I can confirm that the latest release sphinx_hoverxref-1.1.3 works perfectly! 😃

humitos · 2022-09-04T14:01:57Z

Great! Happy to read that 💪

fallback mechanism for inventory reftype meth

942991f

humitos reviewed Sep 20, 2021

View reviewed changes

humitos added a commit that referenced this pull request Sep 23, 2021

reftype fallbacks

af03ce8

Borrowed from #144

chrizzFTD added 3 commits October 2, 2021 20:47

Merge branch 'master' into intersphinx_fallback_inventory_reftype

8c99dd7

# Conflicts: # hoverxref/extension.py

use inventory domain for fallback key roles

a05839e

tests for intersphinx mappings to include functions and methods

7f66ffd

chrizzFTD changed the title ~~Intersphinx - RefType fallback mechanism for inventory reftype meth~~ Intersphinx - RefType - Use objtypes inventory as fallback mechanism Oct 2, 2021

chrizzFTD commented Oct 2, 2021

View reviewed changes

chrizzFTD requested a review from humitos October 3, 2021 00:42

humitos reviewed Oct 6, 2021

View reviewed changes

humitos closed this Jun 14, 2022

chrizzFTD added a commit to thegrill/grill that referenced this pull request Sep 4, 2022

remove custom sphinx-hoverxref from readthedocs/sphinx-hoverxref#144

07210da

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intersphinx - RefType - Use objtypes inventory as fallback mechanism #144

Intersphinx - RefType - Use objtypes inventory as fallback mechanism #144

chrizzFTD commented Sep 19, 2021 •

edited

Loading

astrojuanlu commented Sep 19, 2021

humitos left a comment

chrizzFTD commented Sep 22, 2021

humitos commented Sep 23, 2021

humitos commented Sep 28, 2021

chrizzFTD left a comment •

edited

Loading

chrizzFTD Oct 2, 2021

humitos Oct 6, 2021

chrizzFTD Oct 2, 2021

humitos commented Oct 4, 2021

humitos commented Oct 6, 2021

humitos left a comment •

edited

Loading

humitos Oct 6, 2021

chrizzFTD Oct 11, 2021

humitos Oct 6, 2021

humitos Oct 6, 2021

chrizzFTD commented Oct 11, 2021

humitos commented May 30, 2022

humitos commented Jun 14, 2022

chrizzFTD commented Sep 4, 2022

humitos commented Sep 4, 2022

	for inventory_name in app.config.hoverxref_intersphinx:
	inventory = inventories.named_inventory.get(inventory_name, {})


		:py:meth:`collections.Counter.elements` to test intersphinx methods.

		:py:func:`sum` to test intersphinx functions.

	keys_to_check = [reftype, *env_domain.objtypes_for_role(reftype)]
	keys_to_check = set([reftype, *env_domain.objtypes_for_role(reftype)])

Intersphinx - RefType - Use objtypes inventory as fallback mechanism #144

Intersphinx - RefType - Use objtypes inventory as fallback mechanism #144

Conversation

chrizzFTD commented Sep 19, 2021 • edited Loading

Issue

Discoveries

Proposal

astrojuanlu commented Sep 19, 2021

humitos left a comment

Choose a reason for hiding this comment

chrizzFTD commented Sep 22, 2021

humitos commented Sep 23, 2021

humitos commented Sep 28, 2021

chrizzFTD left a comment • edited Loading

Choose a reason for hiding this comment

chrizzFTD Oct 2, 2021

Choose a reason for hiding this comment

humitos Oct 6, 2021

Choose a reason for hiding this comment

chrizzFTD Oct 2, 2021

Choose a reason for hiding this comment

humitos commented Oct 4, 2021

humitos commented Oct 6, 2021

humitos left a comment • edited Loading

Choose a reason for hiding this comment

humitos Oct 6, 2021

Choose a reason for hiding this comment

chrizzFTD Oct 11, 2021

Choose a reason for hiding this comment

humitos Oct 6, 2021

Choose a reason for hiding this comment

humitos Oct 6, 2021

Choose a reason for hiding this comment

chrizzFTD commented Oct 11, 2021

humitos commented May 30, 2022

humitos commented Jun 14, 2022

chrizzFTD commented Sep 4, 2022

humitos commented Sep 4, 2022

chrizzFTD commented Sep 19, 2021 •

edited

Loading

chrizzFTD left a comment •

edited

Loading

humitos left a comment •

edited

Loading