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

Fix #3351: intersphinx does not refers context #3425

Merged
merged 2 commits into from
Apr 23, 2017

Conversation

tk0miya
Copy link
Member

@tk0miya tk0miya commented Feb 15, 2017

refs: #3351

I added a new interface Domain.get_full_qualified_name() to get the full qualified name from the node.
It allows to refer a target using context (e.g. py:module).

@tk0miya tk0miya self-assigned this Feb 15, 2017
@tk0miya tk0miya added this to the 1.6 milestone Feb 15, 2017
@tk0miya tk0miya requested a review from shimizukawa February 15, 2017 16:51
@bitprophet
Copy link
Contributor

A couple random suggestions:

  • Given the implemented behavior is backwards compatible (the fully-qualified name is only added to the to_try list if it's non-empty), I'd argue there is no need to implement it for all other domains before merging; that could be left for future PRs from interested users.
  • I wonder if it's possible to simply reuse the machinery that implements the 'find it anywhere' behavior from py:obj, i.e. the code that knows .ClassName can refer to anything called ClassName within the current project, regardless of module scope.
    • This is partly because I'm not 100% sure the current code will work right; when I apply it to an intersphinx reference, it would attempt to look up the name as if it was defined in the referencing module and class instead of the module and class it truly lives in.
    • So for example, referencing remote.project.FooClass inside a docstring from local.project.BarClass would try to add the name local.project.BarClass.FooClass to the lookup list...which I don't think would exist in the inventory for remote?

@tk0miya tk0miya force-pushed the 3351_intersphinx_ref_with_context branch from 0779cae to b39b019 Compare April 16, 2017 09:29
@tk0miya
Copy link
Member Author

tk0miya commented Apr 16, 2017

Now I rebased to master HEAD. Unfortunately, I don't have enough time to improve this to all of domains. so I'll do that in future version (@bitprophet Thank you for advice, I've overlooked your comment longer...).

@shimizukawa could you review this please? This adds a new interface to Domain class. So it should be reviewed before merging.

@tk0miya
Copy link
Member Author

tk0miya commented Apr 16, 2017

So for example, referencing remote.project.FooClass inside a docstring from local.project.BarClass would try to add the name local.project.BarClass.FooClass to the lookup list...which I don't think would exist in the inventory for remote?

@bitprophet Did you mean like following case?

.. py:module:: local.project

.. py:class:: BarClass

   This class refers :py:class:`remote.project.FooClass`.

In this case, intersphinx tries to search both FooClass and local.project.BarClass.remote.project.FooClass from remote inventories (after this PR).

Surely, the next case searches both FooClass and local.project.BarClass.FooClass. I think this is correct behavior of intersphinx.

.. py:module:: local.project

.. py:class:: BarClass

   This class refers :py:class:`FooClass`.

@tk0miya tk0miya changed the title [WIP] Fix #3351: intersphinx does not refers context Fix #3351: intersphinx does not refers context Apr 16, 2017
@bitprophet
Copy link
Contributor

In this case, intersphinx tries to search both FooClass and local.project.BarClass.remote.project.FooClass from remote inventories (after this PR).

If that's the case then I misunderstood something, I thought it was only going to search for the fully-qualified local.project.BarClass.remote.project.FooClass. So, I'll wait & see if this gets merged & try it out. Thanks :)

@tk0miya
Copy link
Member Author

tk0miya commented Apr 18, 2017

Ah, sorry. my explanation was wrong.
Both remote.project.FooClass and local.project.BarClass.remote.project.FooClass are correct.
With this PR, intersphinx tries to search using both original keyword and full qualified one.

@tk0miya tk0miya assigned shimizukawa and unassigned tk0miya Apr 18, 2017
Copy link
Member

@shimizukawa shimizukawa left a comment

Choose a reason for hiding this comment

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

LGTM!

I haven't seen generated doc, but new API should be described by autodoc, right?

@shimizukawa shimizukawa assigned tk0miya and unassigned shimizukawa Apr 19, 2017
@tk0miya
Copy link
Member Author

tk0miya commented Apr 19, 2017

Thank you for reviewing.
I will merge this after discussion in #3630 is finished.

@tk0miya tk0miya merged commit bd7fccb into sphinx-doc:master Apr 23, 2017
@tk0miya tk0miya deleted the 3351_intersphinx_ref_with_context branch April 23, 2017 07:15
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants