Autosummary additional links for inherited methods and attributes #11712

satr-cowi · 2023-10-06T15:47:44Z

Adds additional variables that can be used in autosummary templates for while also then splitting out methods and attributes if this is the preferred way of using.

Feature or Bugfix

Feature

Purpose

The "path" is collected for the class from which this member is inherited, and then the name of the inherited method or attribute is appended to this depending on which it is.

Can then be used in a template like so:

.. rubric:: {{ _('Inherited Methods') }}
.. autosummary::
	{% for item in inherited_methods %}
	{{item}}
	{%- endfor %}

Links then appear to where the method/attribute is actually defined.

Snapshot from the end result of my docs which use this looks like so:

This is my first time contributing to a major opensource package, so do let me know if I've missed something or anything looks off.

Explanations of inherited_paths, inherited_methods and inherited_attributes

…r_inherited_methods_and_attributes

picnixz

You'll need a CHANGES entry when the PR is ready.
You'll need to add tests.

doc/usage/extensions/autosummary.rst

picnixz · 2023-10-08T15:20:51Z

sphinx/ext/autosummary/generate.py

        ns['methods'], ns['all_methods'] = \
            _get_members(doc, app, obj, {'method'}, include_public={'__init__'})
        ns['attributes'], ns['all_attributes'] = \
            _get_members(doc, app, obj, {'attribute', 'property'})

+        method_string = [m.split(".")[-1] for m in ns['methods']]


Perhaps it's better to have use ns['all_methods'] since otherwise you'll only include public ones.

@satr-cowi Have you addressed this issue?

I think my preference is for ns['inherited_methods'] would be to only return the public methods (and therefore be consistent with ns['methods'])

We could introduce ns['all_inherited_methods'] as well, but the documentation doesn't include anything about ns['all_methods'] at the moment so we should probably include that too if we do this.

Personally, I want us to be consistent without breaking previous changes.

AFAIU the current behaviour is "ns[C] returns public members for category C" and "ns[all_C] returns all members for category C (including private ones)". I don't mind not documenting "all_*" for now, but I would like the documentation for ns["members"] & co. to be updated then. Currently it says (for "members"):

List containing names of all members of the module or class. Only available
for modules and classes.

In addition, your "inherited_qualnames" documentation tells me about:

members returns a list with many built in methods/attributes and
['__init__', '_i_am_private', 'do_something', 'do_something_else', 'foo_attr']

So now your members also include private members.

Current behaviour (as in the example) is:

methods and attributes returns just public methods/attributes

members and inherited members return all members including private

all_methods and all_attributes returns all methods/attributes including private, but these are not documented so most users won't know they exist

I've updated the "members" documentation to make it clear that this and "inherited members" do include private members. It was already clear that methods/attributes were only public.

The proposed additions are just that:

inherited_methods and inherited_attributes returns just public inherited methods/attributes

inherited_qualnames is the same as inherited_members (includes private), but just with the fully qualified names

Think this is pretty clear from the documentation now

sphinx/ext/autosummary/generate.py

Change name to qualnames Re-use previously defined inherited members Include private in qualnames, but still not in methods/attributes

satr-cowi · 2023-10-09T12:03:30Z

Hi @picnixz, thanks for your help, the comments were really useful. I've made an example that I used in the updated documentation and can turn into a unit test but want to make sure you are happy with the following first:

I think that inherited_methods and inherited_attributes should reflect the same as methods and attributes by only returning "puplic" methods/attributes for consistency. We could add in inherited_all_methods and inherited_all_attributes too pretty simply, but I'm not sure how big the need is, with the fact that all_methods isn't currently documented (a user can always just filter qualnames themselves using these too)?

The dict part has been directly taken from the current implementation of inherited_members, so therefore this will not be a breaking change in any way (as this bit wouldn't currently work for classes without a dict anyway). I couldn't figure out how to use _get_members() to only get the local members of a particular class, so have left it how it is for now. Feel free to update if you know a better way but my knowledge of Python inheritance isn't that deep!

picnixz · 2023-10-09T13:05:58Z

I won't have time this week for any in depth review (especially on autodoc-related topics) so it needs to wait until next week or the week after.

…attributes' of https://github.com/satr-cowi/sphinx into Autosummary_additional_links_for_inherited_methods_and_attributes

Attributes now also include attributes defined in _init_ like in test

… __init__

satr-cowi · 2023-10-24T11:00:47Z

Hi @picnixz, I believe this is ready to go, I've taken on board the feedback and tried to make better use of get_class_members to collect the methods and attributes.

I added a unit test to one of the existing setups, there is also a simple example in the documentation showing the differences between the various variables that can be accessed.

picnixz · 2023-10-24T11:04:43Z

Hi, I am sorry but I don't have time to do anything these weeks. Also I think Adam is also very busy and it doesn't seem to me that we'll push 7.3 before some time. At least the repo is stalled for the past two weeks.

Don't worry about your PR being forgotten since I still watch the repo. However, for the moment being, I won't be able to review your PR :/

satr-cowi · 2023-10-24T11:06:29Z

Ok, no worries!

…r_inherited_methods_and_attributes

…ional_links_for_inherited_methods_and_attributes

…_methods_and_attributes

picnixz · 2024-06-13T14:19:46Z

sphinx/ext/autosummary/generate.py

        ns['methods'], ns['all_methods'] = \
            _get_members(doc, app, obj, {'method'}, include_public={'__init__'})
        ns['attributes'], ns['all_attributes'] = \
            _get_members(doc, app, obj, {'attribute', 'property'})

+        method_string = [m.split(".")[-1] for m in ns['methods']]


@satr-cowi Have you addressed this issue?

picnixz · 2024-06-13T14:22:34Z

tests/test_extensions/test_ext_autosummary.py

+    import autosummary_dummy_inherited_module
+    template = Mock()
+
+    generate_autosummary_content('autosummary_dummy_inherited_module.InheritedAttrClass',


For this one, I would like to have a much wider test coverage. More precisely, I want to be able to test the behaviour of private names, mangled names, inherited methods that, static methods. For that, I would usggest creating a separate dummy module with multiple classes inside. I also want to see how multiple inheritance is handled and how shadowing names are done.

Most of the time, our tests cover simple cases which are fine but everytime there is an issue with autodoc, it's always because of some weird constructions. So I'd like to at least cover non-trivial cases (without trying really pathological cases).

Sure thing, I might need to lean a bit about these more niche cases but happy to give it a first go!

Have made a module where I have tried to test some of these features. Let me know if there is anything specific/additional that you think is missing.

picnixz · 2024-06-13T14:24:13Z

tests/test_extensions/test_ext_autosummary.py

+    assert context['module'] == 'autosummary_dummy_inherited_module'
+    assert context['objname'] == 'InheritedAttrClass'
+    assert context['name'] == 'InheritedAttrClass'
+    assert context['objtype'] == 'class'


Fully qualified names should also be tested (I don't see them but maybe I missed them?)

The full qualified name of this class is included as 'fullname', and the fully qualified name of the inherited members are in 'inherited_qualnames' (lines 383-389)

Oh my bad ! it didn't see it. Ideally I would prefer an exact match because that way you know whether we correctly picked up inherited members and how shadowed names were done, but I assume that there are too many things in the list.

I have tried including all the members in the new unit test, but you then get failures depending on the varying builtin methods included in different versions of Python (e.g. passes for 3.11 and 3.12, but fails on 3.10 and 3.13 for different reasons). Any thoughts on if there is a better way to deal with this rather than what I did above (i.e. don't worry about these additional items in the test)?

Have sorted using if statements depending on Python version as recommended. Implemented in a way where only small tweaks should be needed for future versions.

picnixz · 2024-06-13T14:25:25Z

do you think we could try and get this in a release at some point?

We can definitely make it in a release because this is something I think that is worthwhile. Many people (including me) asked for having a finer control on autosummary. As for "when", I don't know. By the way, if I do not reply on a PR after I said "i'll look into it", please ping me.

doc/usage/extensions/autosummary.rst

…ional_links_for_inherited_methods_and_attributes

…_methods_and_attributes

satr-cowi · 2024-09-25T16:24:08Z

Hi @picnixz, finally got round to making the requested updates. See my comment above on the failing tests, and do let me know your thoughts on if there is a better way to test including the varying in-built members too for each version of Python.

…_methods_and_attributes

picnixz

Ok, after having digested this PR I have one design concern. Currently members returns all attributes, whether they are private or not. In other words, I'd expect inherited_XXX to return all XXX whether they are private or not (because of the naming). I know that we already discussed the point with the methods not returning everything, but I honestly think we need to make the smallest breaking change as well as the simplest one and the most usable one.

For now, do not change your implementation (I'm just asking around).

In order to only have the public ones, I think it's better to have public_inherited_XXX and inherited_XXX where the former returns public lookalike attributes while the latter returns all XXX attributes, whether they are private or not.

What do you think? I'm sorry for my late review but I've been working more on CPython these past months, hence limiting my contribution to Sphinx itself.

cc @AA-Turner @chrisjsewell

doc/usage/extensions/autosummary.rst

picnixz · 2024-10-28T07:13:09Z

doc/usage/extensions/autosummary.rst

@@ -293,16 +293,106 @@ The following variables are available in the templates:

 .. data:: members

-   List containing names of all members of the module or class.  Only available
-   for modules and classes.
+   List containing names of all members of the module or class including private


Just reconfirm, but this was already the case that it returned private attributes right? since we are doing members = dir(self).

Yes, the existing attributes members and private members already included private ones. Therefore there shouldn't be any change here, just clarifying this in the docs.

picnixz · 2024-10-28T07:19:41Z

For instance, since we are adding new features, we could name them more precisely, namely inherited_public_methods instead of inherited_methods. Or we can continue doing what we were doing with methods and go (in the future) for the all_inherited_methods route.

I like having the public in the finer-grained feature because you don't forget about it. On the other hand, not having the all is not necessarily bothersome.

Finally, can you fix the tests as well please? thank you in advance!

satr-cowi · 2024-10-28T07:37:24Z

For instance, since we are adding new features, we could make name them more precisely, namely inherited_public_methods instead of inherited_methods. Or we can continue doing what we were doing with methods and go (in the future) for the all_inherited_methods route.

I like having the public in the finer-grained feature because you don't forget about it. On the other hand, not having the all is not necessarily bothersome.

I also like the clarity of stating 'public', but would like to keep consistency. Ideally we would also update 'methods' to be 'public_methods' too. Could perhaps add this as an equivalent to 'methods', but leave in the original so it is not breaking? Can state it as deprecated in the docs?

If we are doing this, perhaps we should start to document 'all_methods'/'all_inherited_methods' etc. too?

Finally, can you fix the tests as well please? thank you in advance!

Might not have commented properly on this one, but I did have a question here. Different Python versions have different in built methods. You asked above to include these in the tests, rather than just checking that the expected values are in the result. edit: Found the comment - https://github.com/sphinx-doc/sphinx/pull/11712/files#r1775521600

Do you have a good recommendation for testing against each of the expected inbuilt methods for different python versions?

picnixz · 2024-10-28T07:52:22Z

Do you have a good recommendation for testing against each of the expected inbuilt methods for different python versions?

Somehting like this:

if sys.version_info[:2] >= (3, 14):
	expected_builtins = [...]
elif sys.version_info[:2] >= (3, 13):
	expected_builtins = [...]
elif sys.version_info[:2] >= (3, 12):
	expected_builtins = [...]

possibly with finer-grained checks depending on whether a built-in has been added or not (up to you for creating those ifs so that it's the most readable and simple)

I also like the clarity of stating 'public', but would like to keep consistency. Ideally we would also update 'methods' to be 'public_methods' too. Could perhaps add this as an equivalent to 'methods', but leave in the original so it is not breaking? Can state it as deprecated in the docs?

We will need to leave it as it is, possibly adding a DeprecationWarning saying that it will return all methods in the future and that public methods will be exposed as public_methods instead. But I'll wait for Adam's and Chris' opinion on that matter.

If we are doing this, perhaps we should start to document 'all_methods'/'all_inherited_methods' etc. too?

We can do it in a separate PR I think.

satr-cowi · 2024-10-29T08:53:48Z

Tests are sorted to include the expected builtins for multiple Python versions.

Let me know once we have a conclusion on how we want to implement the naming of public/private members and can change as necessary.

Will need to update the version numbers I've put above, and add to the changelog, but can do this when ready to merge.

picnixz · 2024-10-29T17:21:17Z

sphinx/ext/autosummary/generate.py

@@ -398,9 +427,12 @@ def _skip_member(app: Sphinx, obj: Any, name: str, objtype: str) -> bool:
        return False


-def _get_class_members(obj: Any) -> dict[str, Any]:
+def _get_class_members(obj: Any, return_object: bool = True) -> dict[str, Any]:


I would actually flip the boolean and use return_types=False by default. What do you think?

And whether we change the boolean or not, I think it would be good to make it keyword-only just so that we know what we do when we call the function (we don't need to go to its implementation).

picnixz · 2024-10-29T17:22:46Z

tests/roots/test-ext-autosummary/autosummary_dummy_complex_inheritance_module.py

+    def get_age(self):
+        return f"Parent class -  {self.age}"
+
+    # Test mangled name behaviour


Can we have a normal private attribute/method (no mangled, just simple underscore)?

picnixz · 2024-10-29T17:23:47Z

tests/roots/test-ext-autosummary/autosummary_dummy_complex_inheritance_module.py

+# The following is used to process the expected builtin members for different
+# versions of Python. The base list is for v3.11 (the latest testing when added)
+# and new builtin attributes/methods in later versions can be appended below.


Can we have those values declared in the test instead? the test roots should be only contains the code we "test" (so this file should be like a real one on which you run autodoc).

picnixz · 2024-10-29T17:24:29Z

tests/test_extensions/test_ext_autosummary.py

+    assert context['all_methods'] == [
+        '__delattr__',
+        '__dir__',
+        '__eq__',
+        '__format__',
+        '__ge__',
+        '__getattribute__',
+        '__getstate__',
+        '__gt__',
+        '__hash__',
+        '__init__',
+        '__init_subclass__',
+        '__le__',
+        '__lt__',
+        '__ne__',
+        '__new__',
+        '__reduce__',
+        '__reduce_ex__',
+        '__repr__',
+        '__setattr__',
+        '__sizeof__',
+        '__str__',
+        '__subclasshook__',


Aren't those methods inherited from object? can't they be automatically detected like this?

picnixz · 2024-10-29T17:25:09Z

tests/test_extensions/test_ext_autosummary.py

+        'builtins.object.__class__',
+        'builtins.object.__delattr__',
+        'builtins.object.__dir__',
+        'builtins.object.__eq__',
+        'builtins.object.__format__',
+        'builtins.object.__ge__',
+        'builtins.object.__getattribute__',
+        'builtins.object.__getstate__',
+        'builtins.object.__gt__',
+        'builtins.object.__hash__',
+        'builtins.object.__init_subclass__',
+        'builtins.object.__le__',
+        'builtins.object.__lt__',
+        'builtins.object.__ne__',
+        'builtins.object.__new__',
+        'builtins.object.__reduce__',
+        'builtins.object.__reduce_ex__',
+        'builtins.object.__repr__',
+        'builtins.object.__setattr__',
+        'builtins.object.__sizeof__',
+        'builtins.object.__str__',
+        'builtins.object.__subclasshook__',
+    ]


Can't we generate the builtins.object.* methods using [f'builtins.object.{name}' for name in dir(object)]?

satr-cowi and others added 4 commits August 22, 2023 16:39

Add links for inherited methods and attributes

e62a37c

Docs update for additional autosummary inherited options

4e71d5b

Explanations of inherited_paths, inherited_methods and inherited_attributes

Merge branch 'sphinx-doc:master' into Autosummary_additional_links_fo…

6a2b58a

…r_inherited_methods_and_attributes

Formatting update for linter

2f4d125

picnixz reviewed Oct 8, 2023

View reviewed changes

satr-cowi added 3 commits October 9, 2023 12:44

Review comment changes

bee1d6b

Change name to qualnames Re-use previously defined inherited members Include private in qualnames, but still not in methods/attributes

Autosummary documentation improvements

4302a97

Docs linter updates

092f706

Update autosummary.rst

063d33f

satr-cowi added 11 commits October 9, 2023 15:39

Change inherited_members into ordered list

b8dff3f

Added unittest to existing tests for inherited methods and attributes

697be33

Merge branch 'Autosummary_additional_links_for_inherited_methods_and_…

76c0194

…attributes' of https://github.com/satr-cowi/sphinx into Autosummary_additional_links_for_inherited_methods_and_attributes

lint formatting

192ad95

Removed use of __dict__

e9edee7

Attributes now also include attributes defined in _init_ like in test

Test shouldn't depend on Python version or os. Includes attributes in…

b7a0049

… __init__

docs updated

0901005

Linting corrections

de412e0

correction

4f3aa01

another linting correction

8e62aed

Refactor to minimise _get_class_members calls

71c1729

satr-cowi and others added 5 commits November 30, 2023 12:20

Merge branch 'sphinx-doc:master' into Autosummary_additional_links_fo…

89adbd9

…r_inherited_methods_and_attributes

Merge remote-tracking branch 'upstream/master' into Autosummary_addit…

2d56014

…ional_links_for_inherited_methods_and_attributes

Merge remote-tracking branch 'upstream/master' into Autosummary_addit…

bff342f

…ional_links_for_inherited_methods_and_attributes

Merge branch 'master' into Autosummary_additional_links_for_inherited…

13895a9

…_methods_and_attributes

Merge branch 'master' into Autosummary_additional_links_for_inherited…

7f06fcb

…_methods_and_attributes

picnixz reviewed Jun 13, 2024

View reviewed changes

picnixz reviewed Jun 14, 2024

View reviewed changes

doc/usage/extensions/autosummary.rst Show resolved Hide resolved

satr-cowi and others added 8 commits September 23, 2024 17:57

Merge remote-tracking branch 'upstream/master' into Autosummary_addit…

219cc23

…ional_links_for_inherited_methods_and_attributes

ruff format

3199fe0

Only return the first instance of inheritance

9884579

Additional tests for more conditions

5421b26

Clarification on members including private and template example

a34b648

ruff format

6f3fbc3

lint corrections

9037a22

Merge branch 'master' into Autosummary_additional_links_for_inherited…

224076b

…_methods_and_attributes

picnixz self-requested a review September 26, 2024 17:07

Merge branch 'master' into Autosummary_additional_links_for_inherited…

065a1a0

…_methods_and_attributes

picnixz reviewed Oct 28, 2024

View reviewed changes

satr-cowi added 10 commits October 29, 2024 11:18

add in version dependent attributes

58ff1e3

ruff

3c9b0bb

update for 3_11 and remove extra term

336d3e3

add in members too

ceb2b99

Simplify way to add new builtin members

479ff7d

tweak for 3_14 test

4987598

fix 3_14

a3adf19

correct order of members2

d6abcbe

Improved comment

73827e2

Improved docstring for inherited_qualnames

242f21a

picnixz reviewed Oct 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autosummary additional links for inherited methods and attributes #11712

Autosummary additional links for inherited methods and attributes #11712

satr-cowi commented Oct 6, 2023 •

edited

Loading

picnixz left a comment

picnixz Oct 8, 2023

picnixz Jun 13, 2024

satr-cowi Jun 14, 2024

picnixz Jun 14, 2024

satr-cowi Sep 25, 2024

satr-cowi commented Oct 9, 2023

picnixz commented Oct 9, 2023

satr-cowi commented Oct 24, 2023

picnixz commented Oct 24, 2023 •

edited

Loading

satr-cowi commented Oct 24, 2023

picnixz Jun 13, 2024

picnixz Jun 13, 2024

satr-cowi Jun 14, 2024

satr-cowi Sep 25, 2024

picnixz Jun 13, 2024

satr-cowi Jun 14, 2024

picnixz Jun 14, 2024

satr-cowi Sep 25, 2024

satr-cowi Oct 29, 2024

picnixz commented Jun 13, 2024 •

edited

Loading

satr-cowi commented Sep 25, 2024

picnixz left a comment •

edited

Loading

picnixz Oct 28, 2024

satr-cowi Oct 28, 2024

picnixz commented Oct 28, 2024 •

edited

Loading

satr-cowi commented Oct 28, 2024 •

edited

Loading

picnixz commented Oct 28, 2024 •

edited

Loading

satr-cowi commented Oct 29, 2024

picnixz Oct 29, 2024 •

edited

Loading

picnixz Oct 29, 2024 •

edited

Loading

picnixz Oct 29, 2024

picnixz Oct 29, 2024

picnixz Oct 29, 2024

Autosummary additional links for inherited methods and attributes #11712

Are you sure you want to change the base?

Autosummary additional links for inherited methods and attributes #11712

Conversation

satr-cowi commented Oct 6, 2023 • edited Loading

Feature or Bugfix

Purpose

picnixz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satr-cowi commented Oct 9, 2023

picnixz commented Oct 9, 2023

satr-cowi commented Oct 24, 2023

picnixz commented Oct 24, 2023 • edited Loading

satr-cowi commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz commented Jun 13, 2024 • edited Loading

satr-cowi commented Sep 25, 2024

picnixz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz commented Oct 28, 2024 • edited Loading

satr-cowi commented Oct 28, 2024 • edited Loading

picnixz commented Oct 28, 2024 • edited Loading

satr-cowi commented Oct 29, 2024

picnixz Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

picnixz Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satr-cowi commented Oct 6, 2023 •

edited

Loading

picnixz commented Oct 24, 2023 •

edited

Loading

picnixz commented Jun 13, 2024 •

edited

Loading

picnixz left a comment •

edited

Loading

picnixz commented Oct 28, 2024 •

edited

Loading

satr-cowi commented Oct 28, 2024 •

edited

Loading

picnixz commented Oct 28, 2024 •

edited

Loading

picnixz Oct 29, 2024 •

edited

Loading

picnixz Oct 29, 2024 •

edited

Loading