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

Autosummary: Extend __all__ members to template rendering #10811

Merged
merged 7 commits into from Apr 5, 2023

Conversation

ClementPinard
Copy link
Contributor

Fixes #10809

  • the option autosummary_ignore_module_all when set to False adds members to module's members entry that will be used for autodoc, but otherwise it ignores it. As such, if a class is available in the __all__, it won't be generated.
  • This commit aims at extending the __all__ handling not only to members, but also to corresponding attribute types (functions, classes, exceptions, modules)
  • In short, the imported_members option is set to True if the object has __all__ member and autosummary is set to have autosummary_ignore_module_all set to False

See an example repo where the behaviour is changed : https://github.com/ClementPinard/sphinx_autosummary_bug_example

this PR is (at least initially) just for illustration and discussion purpose. tests where not checked

Feature or Bugfix

  • Feature
  • Bugfix

A mix of both

Detail

Fixes sphinx-doc#10809

* the option autosummary_ignore_module_all when set to False adds members to module's members entry that will be used for autodoc, but otherwise it ignores it. As such, if a class is available in the __all__, it won't be generated.
* This commit aims at extending the __all__ handling not only to members, but also to corresponding attribute types (function, classes, exceptions, modules)
* In short, the imported_members option is set to True if the object has __all__ member and autosummary is set to have autosummary_ignore_module_all set to False
@ClementPinard ClementPinard changed the base branch from master to 6.1.x April 4, 2023 13:16
@ClementPinard ClementPinard changed the base branch from 6.1.x to master April 4, 2023 13:16
@ClementPinard
Copy link
Contributor Author

Hi, is this improvement proposition considered ? I have created this PR as a draft to spark some discussion about changing autosummary behaviour, but otherwise it could be reviewed as is.

I really think the current behaviour is not one users would expect when they use the recursive option.

@ClementPinard ClementPinard marked this pull request as ready for review April 4, 2023 13:25
@AA-Turner
Copy link
Member

Please may you add a test of the new behaviour, and an entry to the changelog (CHANGES)?

A

@AA-Turner AA-Turner changed the title extend __all__ members to template rendering Autosummary: Extend __all__ members to template rendering Apr 4, 2023
@ClementPinard
Copy link
Contributor Author

Thanks for the feedback :)

here it is with test and CHANGES entry.

Note that for now it does not take into account a module that would be in __all__ but not explicitly imported. It's a problem because it can be imported and thus should be documented. And it does work when there is no __all__ in the package __init__.py.

So it still lacks flexibility, but at least, it's better than before 😛

* When ignore_module_all is False, search for imported modules (possibly renamed) and in discovered modules, public modules are then the one mentioned in __all__
* In a more general usecase, skip all modules that are overwritten in the package namespace
@ClementPinard
Copy link
Contributor Author

I think I have found a solution that works.

If __all__ is given , first look for imported modules (possibly renamed) and then look for the rest.

Additionally, there is now a skip list for get_modules, to avoid tryinbg to get documentation from a module that was overwritten in the package namespace

This will cause this kind of code to not crash with sphinx :

(file package/__init__.py)

from .a import a

autosummary will now know that package.a is a function and not a module anymore.

NB: Yes this does happen even though I would personally consider it an antipattern. Some JAVA inspired python code exist out there with only a class in the module and a class the same name of the module and then an import similar to the aforementionned one in the parent package __init__.py. Whether we should try to be compliant with this or not is debatable but it it's possible to do that in python, so it will not disappear.

On a more genral note, I have seen that the difference between private members and public members is the leading underscore (see https://github.com/sphinx-doc/sphinx/blob/master/sphinx/ext/autosummary/generate.py#L283).

Without a proper __all__ list it makes sense, but if it is given (and the autosummary_ignore_module_all is False), I think it should prevail as it is by definition the public API. It's out of the scope of this PR, but I can add this modification as well if you want.

@AA-Turner AA-Turner merged commit 9299003 into sphinx-doc:master Apr 5, 2023
22 checks passed
@AA-Turner
Copy link
Member

Thank you @ClementPinard!

A

@AA-Turner AA-Turner added this to the 6.2.0 milestone Apr 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autosummary: autosummary_ignore_module_all does not work as documented
2 participants