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

use the obj role for all See Also items #8051

Merged
merged 5 commits into from Oct 29, 2020
Merged

Conversation

keewis
Copy link
Contributor

@keewis keewis commented Aug 4, 2020

Feature or Bugfix

  • Bugfix

Purpose

Before this change, napoleon would look for See Also items using the same role as the documented object. This breaks once one tries to link to other kinds of objects, e.g. from a function to a method.

With the proposed changes, the code parsing See Also sections will try to find the correct role for a referenced object from the intersphinx inventory.

Relates

@keewis
Copy link
Contributor Author

keewis commented Aug 6, 2020

the changes I just pushed will try to apply napoleon_type_aliases to the See Also section. This would allow me to manually rewrite unprefixed methods like Index.any to pandas.Index.any, which might be a solution for the namespacing issues I described in #7690. Not sure if that's the best option to fix that issue, though?

@tk0miya tk0miya added this to the 3.3.0 milestone Aug 7, 2020
@tk0miya
Copy link
Member

tk0miya commented Aug 7, 2020

Before this change, napoleon would look for See Also items using the same role as the documented object. This breaks once one tries to link to other kinds of objects, e.g. from a function to a method.

I agree this is an incorrect approach. +1 for changing this behavior. I don't know why the original code chooses a role from self._what.

But it's too much to access the internal data structure of intersphinx directly. I think it is better to use :py:obj: always instead. Then all trouble will be resolved, right?

@keewis
Copy link
Contributor Author

keewis commented Aug 7, 2020

that does work and is indeed simpler. However, func and meth display the link slightly different (they append ()), probably not an issue, though?

@tk0miya
Copy link
Member

tk0miya commented Aug 7, 2020

Surely. But I doubt that all items in "see also" field for methods (or functions) always refer to methods (or functions). So it would be better to use :obj: reference instead. Users can use :meth: and :func: roles explicitly for each item if they need to refer it as callable.

My main concern is such change is an incompatible change or not. @shimizukawa do you have an opinion?

@keewis keewis changed the title search the role for See Also items in the intersphinx inventory use the obj role for all See Also items Oct 3, 2020
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

It seems @shimizukawa has been busy. Okay, let's merge this.

@@ -1151,6 +1151,41 @@ def push_item(name: str, rest: List[str]) -> None:
items.append((name, list(rest), role))
del rest[:]

def search_inventory(inventory, name, hint=None):
Copy link
Member

Choose a reason for hiding this comment

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

This function is no longer needed now.

else:
link = "`%s`_" % func
link = ':%s:`%s`' % (func_role, func)
Copy link
Member

Choose a reason for hiding this comment

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

It seems func_role would not be changed. How about embedding it into the format-string?

@tk0miya
Copy link
Member

tk0miya commented Oct 29, 2020

I'll update this code after merging.

@tk0miya tk0miya merged commit 921f097 into sphinx-doc:3.x Oct 29, 2020
@tk0miya
Copy link
Member

tk0miya commented Oct 29, 2020

Merged. Thank you for your contribution!

tk0miya added a commit that referenced this pull request Oct 29, 2020
@keewis keewis deleted the fix-see_also branch October 29, 2020 15:53
@tk0miya tk0miya mentioned this pull request Oct 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants