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

Changes from 1 commit
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
11 changes: 6 additions & 5 deletions 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 @@ -192,7 +193,7 @@ def is_skipped(self, name: str, value: Any, objtype: str) -> bool:
name, exc, type='autosummary')
return False

def scan(self, imported_members: bool) -> List[str]:
def scan(self, imported_members: bool, attr_docs: Mapping[str,str] = {}) -> List[str]:
members = []
for name in members_of(self.object, self.app.config):
try:
Expand All @@ -208,7 +209,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 Expand Up @@ -305,8 +306,6 @@ def get_module_attrs(members: Any) -> Tuple[List[str], List[str]]:
"""Find module attributes with docstrings."""
attrs, public = [], []
try:
analyzer = ModuleAnalyzer.for_module(name)
attr_docs = analyzer.find_attr_docs()
for namespace, attr_name in attr_docs:
if namespace == '' and attr_name in members:
attrs.append(attr_name)
Expand Down Expand Up @@ -335,8 +334,10 @@ def get_modules(obj: Any) -> Tuple[List[str], List[str]]:
ns.update(context)

if doc.objtype == 'module':
analyzer = ModuleAnalyzer.for_module(name)
attr_docs = analyzer.find_attr_docs()
lokik marked this conversation as resolved.
Show resolved Hide resolved
scanner = ModuleScanner(app, obj)
ns['members'] = scanner.scan(imported_members)
ns['members'] = scanner.scan(imported_members, attr_docs)
ns['functions'], ns['all_functions'] = \
get_members(obj, {'function'}, imported=imported_members)
ns['classes'], ns['all_classes'] = \
Expand Down