Fix Python domain nesting #3520
Conversation
Moved sphinx-doc#3465 here, to address this in `stable` instead. This fixes a problem with the Python domain object nesting. Because only one object name was stored in `ref_context`, and reset to `None` in `after_content`, nesting broke if you put anything after a nested class: ```rst .. py:class:: Parent .. py:method:: foo() This wouldn't resolve: :py:meth:`bar` .. py:class:: Child In the `after_content` method, the object is reset to `None`, so anything after this in the same nesting is considered to be top level instead. .. py:method:: bar() This is top level, as the domain thinks the surrounding object is `None` ``` This depends on sphinx-doc#3519 and can be rebased after that is merged into stable Fixes sphinx-doc#3065 Refs sphinx-doc#3067
agjohnson
force-pushed
the
fix-domain-py-nesting
branch
from
696fc6e
to
b0875d6
Compare
|
Rebased on stable with #3519 merged |
sphinx/domains/python.py
Outdated
| try: | ||
| self.env.ref_context['py:classes'].append(prefix) | ||
| except (AttributeError, KeyError): | ||
| self.env.ref_context['py:classes'] = [prefix] |
There was a problem hiding this comment.
Please use dict.setdefault() instead.
Then you don't need to catch any exceptions:
classes = self.env.ref_context.setdefault('py:classes', [])
classes.append(prefix)
sphinx/domains/python.py
Outdated
| if self.allow_nesting: | ||
| try: | ||
| self.env.ref_context['py:classes'].pop() | ||
| except (KeyError, IndexError): |
There was a problem hiding this comment.
Are these exception raised here?
There was a problem hiding this comment.
Both can, yes. KeyError if py:classes has not been set yet, and IndexError if this is an empty list.
There was a problem hiding this comment.
Oh, sorry. I understand your code now.
Surely, it will be raised in case of no prefix found in before_content().
Okay, no problem.
sphinx/domains/python.py
Outdated
| except IndexError: | ||
| cls_name = None | ||
| finally: | ||
| self.env.ref_context['py:class'] = cls_name |
There was a problem hiding this comment.
I prefer to simple implementation. How about this?
if len(self.env.ref_context['py:classes']) > 0:
self.env.ref_context['py:class'] = self.env.ref_context['py:classes'][-1]
else:
self.env.ref_context['py:class'] = None
There was a problem hiding this comment.
I think the original is more succinct and pythonic, but I don't have a strong opinion. This change looks fine
sphinx/domains/python.py
Outdated
| """ | ||
| prefix = None | ||
| if self.names: | ||
| (cls_name, cls_name_prefix) = self.names.pop() |
There was a problem hiding this comment.
Until now, self.names.pop() is not called.
Is there any side effect?
sphinx/domains/python.py
Outdated
| (cls_name, cls_name_prefix) = self.names.pop() | ||
| prefix = cls_name_prefix.strip('.') if cls_name_prefix else None | ||
| if self.allow_nesting: | ||
| prefix = cls_name |
There was a problem hiding this comment.
prefix is determined by both allow_nesting and cls_name_prefix.
So how about this? I feel this is more simple.
if self.allow_nesting:
prefix = cls_name
elif cls_name_prefix:
prefix = cls_name_prefix.strip('.')
else:
prefix = None
sphinx/domains/python.py
Outdated
| # type: () -> None | ||
| """Handle object nesting before content | ||
|
|
||
| If this class is a nestable object, such as a class object, build up a |
There was a problem hiding this comment.
I feel a bit strange for "this class". How about "this instance" or "the instance"?
There was a problem hiding this comment.
I feel like class is the most correct, as allow_nesting is used as a class variable. I can find a clearer way of describing this though.
There was a problem hiding this comment.
I felt confused the description uses "class" twice. The first class; "this class" means subclasses of PyObject. It is an implementation of directives. so my proposal was wrong.
And second one; "a class object" means subjects of directives. Sometime it is a class, a method, and so on.
I'm not a native English speaker and not good at English. so If you feel this is okay, go ahead with this.
sphinx/domains/python.py
Outdated
| """ | ||
| prefix = None | ||
| if self.names: | ||
| (cls_name, cls_name_prefix) = self.names.pop() |
There was a problem hiding this comment.
Hmm... self.names.pop() contains always "class name" ?
As a quick look, it also contains modules, methods, functions and so on.
If my consideration is true, please rename this to better one.
There was a problem hiding this comment.
That makes sense, I'll update this
agjohnson
force-pushed
the
fix-domain-py-nesting
branch
from
9904941
to
2795889
Compare
|
Things are now cleaned up and rebased |
|
Merged. Thank you for contribution! |
Moved #3465 here, to address this in
stableinstead.This fixes a problem with the Python domain object nesting. Because only
one object name was stored in
ref_context, and reset toNoneinafter_content, nesting broke if you put anything after a nested class:This depends on #3519 and can be rebased after that is merged into stable
Fixes #3065 Refs #3067