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.

Sign up for GitHub

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

Jump to bottom

Recognize a documented attribute of a module as non-imported. #10258

Closed
wants to merge 3 commits into from
Closed

Recognize a documented attribute of a module as non-imported. #10258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
033ebab
Recognize a documented attribute of a module as non-imported.
lokik Mar 11, 2022
d955098
Do not pass attr_docs as an argument into ModuleScanner.scan
lokik Mar 14, 2022
07b2a05
Test recognition of a documented instance of a class as a non-importe…
lokik Mar 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion sphinx/ext/autosummary/generate.py
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@
from gettext import NullTranslations
from os import path
from typing import Any, Dict, List, NamedTuple, Sequence, Set, Tuple, Type, Union
from collections.abc import Mapping

from jinja2 import TemplateNotFound
from jinja2.sandbox import SandboxedEnvironment
Expand Down Expand Up @@ -194,6 +195,8 @@ def is_skipped(self, name: str, value: Any, objtype: str) -> bool:

def scan(self, imported_members: bool) -> List[str]:
members = []
analyzer = ModuleAnalyzer.for_module(self.object.__name__)
attr_docs = analyzer.find_attr_docs()
for name in members_of(self.object, self.app.config):
try:
value = safe_getattr(self.object, name)
Expand All @@ -208,7 +211,7 @@ def scan(self, imported_members: bool) -> List[str]:
if inspect.ismodule(value):
imported = True
elif safe_getattr(value, '__module__') != self.object.__name__:
imported = True
imported = objtype != 'data' or ('', name) not in attr_docs
Copy link
Member

Choose a reason for hiding this comment

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

Is this condition related to the __module__? I think this check should be separated to another condition.

Copy link
Contributor Author

@lokik lokik Mar 15, 2022 •
edited

Choose a reason for hiding this comment

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

I viewed it as "fix" for the the fact, that instances of user defined classes has __module__ property set to the module, where the class is defined, and not to the one where the object is instantiated. So from the original intent of the commit, yes, it is related, as it is an attempt to fix the false positive of the 'is-the-stuff-imported?' test.

But you are right, that you can consider it and define, that non-imported module attributes are just these, that have some docstring and thus made the condition separate (before the currently modified one). However, this IMHO would lead to further change in the autosummary behavior: so far the instantiated object without docstrings that has __module__ set to the module, where they are instantiated (e.g. if their class is defined in the same module) are considered as non-imported - and that would change.

So, this is maybe really a more systematic treating of the problem and thus maybe a better option? However, the resulting criterion would have no connection to "being imported" at all - in this case I would consider an introduction of a new option, something like "autosummary_only_documented_modules_attributes", and to treat the module attributes according to this new option, independently to the autosummary_imported_members option.

Copy link
Member

Choose a reason for hiding this comment

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

In autodoc, global variables having doc-comment are documented without :imported-members: option even if their __module__ value are different because they're considered as "locally defined variables".

class Foo:
    pass


#: a global variable having different __module__
var1 = int

#: a global variable having same __module__
var2 = Foo

So I think it's better to check the member has doc-comment or not first to determine imported flag.

if ('', name) in attr_docs:
  imported = False
elif inspect.ismodule(value):
  ...

else:
imported = False
except AttributeError:
Expand Down
2 changes: 2 additions & 0 deletions tests/roots/test-ext-autosummary/autosummary_class_module.py
View file
Open in desktop
@@ -0,0 +1,2 @@
class Class():
pass
5 changes: 5 additions & 0 deletions tests/roots/test-ext-autosummary/autosummary_dummy_module.py
View file
Open in desktop
@@ -1,5 +1,6 @@
from os import path # NOQA
from typing import Union
from autosummary_class_module import Class

__all__ = [
"CONSTANT1",
Expand Down Expand Up @@ -60,3 +61,7 @@ class _Exc(Exception):
qux = 2
#: a module-level attribute that has been excluded from __all__
quuz = 2

considered_as_imported = Class()
non_imported_member = Class()
""" This attribute has a docstring, so it is recognized as a not-imported member """
28 changes: 16 additions & 12 deletions tests/test_ext_autosummary.py
View file
Open in desktop
Expand Up @@ -218,15 +218,15 @@ def test_autosummary_generate_content_for_module(app):
assert context['members'] == ['CONSTANT1', 'CONSTANT2', 'Exc', 'Foo', '_Baz', '_Exc',
'__all__', '__builtins__', '__cached__', '__doc__',
'__file__', '__name__', '__package__', '_quux', 'bar',
'quuz', 'qux']
'non_imported_member', 'quuz', 'qux']
assert context['functions'] == ['bar']
assert context['all_functions'] == ['_quux', 'bar']
assert context['classes'] == ['Foo']
assert context['all_classes'] == ['Foo', '_Baz']
assert context['exceptions'] == ['Exc']
assert context['all_exceptions'] == ['Exc', '_Exc']
assert context['attributes'] == ['CONSTANT1', 'qux', 'quuz']
assert context['all_attributes'] == ['CONSTANT1', 'qux', 'quuz']
assert context['attributes'] == ['CONSTANT1', 'qux', 'quuz', 'non_imported_member']
assert context['all_attributes'] == ['CONSTANT1', 'qux', 'quuz', 'non_imported_member']
assert context['fullname'] == 'autosummary_dummy_module'
assert context['module'] == 'autosummary_dummy_module'
assert context['objname'] == ''
Expand Down Expand Up @@ -276,7 +276,8 @@ def skip_member(app, what, name, obj, skip, options):
context = template.render.call_args[0][1]
assert context['members'] == ['CONSTANT1', 'CONSTANT2', '_Baz', '_Exc', '__all__',
'__builtins__', '__cached__', '__doc__', '__file__',
'__name__', '__package__', '_quux', 'quuz', 'qux']
'__name__', '__package__', '_quux', 'non_imported_member',
'quuz', 'qux']
assert context['functions'] == []
assert context['classes'] == []
assert context['exceptions'] == []
Expand All @@ -292,18 +293,20 @@ def test_autosummary_generate_content_for_module_imported_members(app):
assert template.render.call_args[0][0] == 'module'

context = template.render.call_args[0][1]
assert context['members'] == ['CONSTANT1', 'CONSTANT2', 'Exc', 'Foo', 'Union', '_Baz',
'_Exc', '__all__', '__builtins__', '__cached__', '__doc__',
'__file__', '__loader__', '__name__', '__package__',
'__spec__', '_quux', 'bar', 'path', 'quuz', 'qux']
assert context['members'] == ['CONSTANT1', 'CONSTANT2', 'Class', 'Exc', 'Foo', 'Union',
'_Baz', '_Exc', '__all__', '__builtins__', '__cached__',
'__doc__', '__file__', '__loader__', '__name__',
'__package__', '__spec__', '_quux', 'bar',
'considered_as_imported', 'non_imported_member', 'path',
'quuz', 'qux']
assert context['functions'] == ['bar']
assert context['all_functions'] == ['_quux', 'bar']
assert context['classes'] == ['Foo']
assert context['all_classes'] == ['Foo', '_Baz']
assert context['classes'] == ['Class', 'Foo']
assert context['all_classes'] == ['Class', 'Foo', '_Baz']
assert context['exceptions'] == ['Exc']
assert context['all_exceptions'] == ['Exc', '_Exc']
assert context['attributes'] == ['CONSTANT1', 'qux', 'quuz']
assert context['all_attributes'] == ['CONSTANT1', 'qux', 'quuz']
assert context['attributes'] == ['CONSTANT1', 'qux', 'quuz', 'non_imported_member']
assert context['all_attributes'] == ['CONSTANT1', 'qux', 'quuz', 'non_imported_member']
assert context['fullname'] == 'autosummary_dummy_module'
assert context['module'] == 'autosummary_dummy_module'
assert context['objname'] == ''
Expand Down Expand Up @@ -351,6 +354,7 @@ def test_autosummary_generate(app, status, warning):
' CONSTANT1\n'
' qux\n'
' quuz\n'
' non_imported_member\n'
' \n' in module)

Foo = (app.srcdir / 'generated' / 'autosummary_dummy_module.Foo.rst').read_text()
Expand Down