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 with custom documenters #7908

Closed
keewis opened this issue Jul 3, 2020 · 21 comments · Fixed by #8038
Closed

Autosummary with custom documenters #7908

keewis opened this issue Jul 3, 2020 · 21 comments · Fixed by #8038
Labels
extensions:autosummary type:enhancement enhance or introduce a new feature

Comments

@keewis
Copy link
Contributor

keewis commented Jul 3, 2020

Is your feature request related to a problem? Please describe.
This is related to #6264, but a bit different (I think?): when using the template option of autosummary directives, autosummary will ignore the template which means it will use the wrong documenter to produce its summary table.

For an example, see sphinx-autosummary-accessors (examples.rst, build)

Describe the solution you'd like
I'd like autosummary to use the documenter specified in the template instead of choosing from the standard documenters.

Describe alternatives you've considered
I considered using the priority to control the directive used but that would mean those directives are always used and not only together with the template.

In keewis/sphinx-autosummary-accessors#6 (keewis/sphinx-autosummary-accessors@aa44bda) I hacked around that by overriding Autosummary.get_items to extract that information from the template and use the correct documenter. That, however, is fragile since now I need to make sure this stays in sync with the upstream method.

@tk0miya
Copy link
Member

tk0miya commented Jul 4, 2020

Sorry. I still don't understand your problem and solution. I think it is hard to look up documented from the chosen template. Why your custom documentor is chosen by autosummary? I need to know your problem first.

@keewis
Copy link
Contributor Author

keewis commented Jul 4, 2020

the problem is this: without the hack (or the feature I'm requesting) autosummary indeed doesn't use the custom documenters at all; the only thing it does with the template is fill it in and write it to disk. That means that it uses the standard documenters to get the signature and summary instead of the custom ones, and the custom documenter is only used by autodoc.

What I'd like autosummary to do is to somehow extract the documenter and the full name from the filled-in template.

If that's something you'd rather not provide directly, I'd like to have a way to implement that without rewriting Autosummary.get_items (and thus requiring a specific sphinx version).

@tk0miya
Copy link
Member

tk0miya commented Jul 5, 2020

AFAIK, Sphinx chooses a documenter for the object from the registry. So your documenter will be chosen automatically if it matches to the object via Documenter.can_document_member() and its priority. I think your problem is that Sphinx does not choose your documenter unexpectedly, right? Did you check why Sphinx did not choose your documenters?

@keewis
Copy link
Contributor Author

keewis commented Jul 5, 2020

it is not chosen because I set the priority lower than MethodDocumenter so that the custom documenter won't be chosen for unrelated functions (I didn't know about can_document_member, but see also below).

For more background: xarray and pandas have a concept called "accessor" where a namespace object is added to group some methods together, e.g. pandas.DataFrame.plot. These should be documented the way they are called: DataFrame.plot.scatter instead of PlotMethods.scatter.

For methods and attributes on the accessor, it doesn't matter if the standard documenters are used: the signature and summary stay the same, regardless of the path (Example.test.multiply vs. TestAccessor.multiply). However, if the accessor is callable, it should be documented differently: the displayed name should be Example.test, not Example.test.__call__, and unless I can change the displayed name I have to reflect that in the autosummary directive:

.. autosummary::
   :template: accessor_callable.rst
  
   Example.test

With the templates, I can get autodoc behave the way I want it to, but this doesn't work with autosummary since it ignores the template.

I'm not sure if I can write a can_document_member that can filter out only the accessor cases since I don't know where I can get the full name from; member is always the object and membername seems to be always empty.

@tk0miya
Copy link
Member

tk0miya commented Jul 5, 2020

Thank you for your explanation. Is your problem resolved if autosummary gives correct membername to can_document_member() method? I consider sphinx.ext.autosummary:get_documenter() has wrong implementation.

@keewis
Copy link
Contributor Author

keewis commented Jul 5, 2020

if there's a way to change the displayed name in the summary table, then yes, probably, although that would mean I'd have to use Example.test.__call__ in the autosummary directive.

The other way this could be solved would be to (additionally?) allow overriding get_documenter, but then I'd also need the Autosummary object to make it useful – maybe consider making it a method?

@tk0miya
Copy link
Member

tk0miya commented Jul 6, 2020

if there's a way to change the displayed name in the summary table

AFAIK, Autosummary.get_items() calls documenter.format_signature() to get a display name for the object via documented. And the method is also used to generate intermediate reST document to generate real contents. I think it can control the generated content.

@keewis
Copy link
Contributor Author

keewis commented Jul 6, 2020

unfortunately, that's not the case: the signature is just the part between the parentheses. This line:

items.append((display_name, sig, summary, real_name))
becomes something like

[('Example.test.multiply', '(factor)', 'multiply data with a factor', 'example.Example.test.multiply')]

where display_name is from here:

display_name = name
if name.startswith('~'):
name = name[1:]
display_name = name.split('.')[-1]
and real_name from here:
with mock(self.config.autosummary_mock_imports):
real_name, obj, parent, modname = import_by_name(name, prefixes=prefixes)
, so I think the documenter currently can't change the name.

@tk0miya
Copy link
Member

tk0miya commented Jul 7, 2020

Oops, it's my bad. You're right. Indeed there are no way to change the name of get_items.

@keewis
Copy link
Contributor Author

keewis commented Jul 7, 2020

then either adding template support to autosummary or allowing to override get_documenter (and passing enough information, e.g. Autosummary.options) could be the way to go?

@tk0miya
Copy link
Member

tk0miya commented Jul 9, 2020

I think a template and documenter is not related at all. So I'll never choose the way. On the other hand, it's not so hard to choose to allow overriding get_documenter() for subclasses. But it does not resolve your case if my understanding correct. It does not mean to change display_name from subclasses.

@keewis
Copy link
Contributor Author

keewis commented Jul 12, 2020

just to be clear, I only need to override display_name if I cannot override get_documenter. So I think that once that is possible (hopefully not by registering the Autosummary subclass using setup.add_directive("autosummary", CustomAutosummary, override=True)), I should be able to get what I want quite easily.

@tk0miya
Copy link
Member

tk0miya commented Jul 14, 2020

I think I don't understand the real problem yet. Why do you rewrite these names on autosummary directive? I feel it would be better to give correct name to the directive on the reST source. Why don't you use DataFrame.plot.scatter instead?

@keewis
Copy link
Contributor Author

keewis commented Jul 14, 2020

that's what I do, and that already works.

However, these namespace objects may also be callable, so I need to make sure __call__ is documented as DataFrame.plot:

.. autosummary::
   :template: autosummary/accessor_callable.rst

   DataFrame.plot

should document DataFrame.plot.__call__, but display it as DataFrame.plot. With autodoc, I can get there with this template:

{{ fullname }}
{{ underline }}

.. currentmodule:: {{ module.split('.')[0] }}

.. autoaccessorcallable:: {{ (module.split('.')[1:] + [objname]) | join('.') }}.__call__

but autosummary ignores the template and thus won't use my documenter. If I could override get_documenter, I could fill in the template, extract the values from currentmodule and autoaccessorcallable and use those and a closure to return the correct documenter, which would fix my issue.

@tk0miya
Copy link
Member

tk0miya commented Jul 14, 2020

but autosummary ignores the template and thus won't use my documenter.

I think this is a real problem. Does autosummary really not use the given template? If so, it's a bug.
Does autosummary really not use your document even if your documenter matches to the object via Documenter.can_document_member()? If so, it's also a bug.

Could you share some examples? I'll try to investigate your problem.

@keewis
Copy link
Contributor Author

keewis commented Jul 14, 2020

take a look at the docs of my extension: The classes are defined in example.py, the autosummary page is examples.rst.

@keewis
Copy link
Contributor Author

keewis commented Jul 24, 2020

gentle ping, @tk0miya.

As a summary, my problem is that autosummary properly forwards the :template: option to autodoc, but won't look at the contents of the file (that's something I think you didn't want to include in autosummary). However, I also can't use can_document_member and the priority attribute to choose the correct documenter since the difference between a callable "accessor" and normal methods is not big enough to make this reliable; the only reliable way I have (or rather, the only way I know of) is using the template option.

So I think allowing to register functions that choose the documenter (with a fall back to the default algorithm if they return NotImplemented) could be the way forward. Does that sound reasonable? If so, I'd be willing to put in a PR that tries to implement that.

@tk0miya
Copy link
Member

tk0miya commented Jul 25, 2020

take a look at the docs of my extension: The classes are defined in example.py, the autosummary page is examples.rst.

Could you describe this to me in detail? Is this an example of a bug that autosummary does not use the given template, or a bug that autosummary does not use your documenter even if it matches via Documenter.can_document_member()? If my understanding correct, your documenters do not implement can_document_member() method. So are you saying the former case?

So I think allowing to register functions that choose the documenter (with a fall back to the default algorithm if they return NotImplemented) could be the way forward. Does that sound reasonable? If so, I'd be willing to put in a PR that tries to implement that.

What is it different fromDocumenter.can_document_member()?

@keewis
Copy link
Contributor Author

keewis commented Jul 25, 2020

Is this an example of a bug that autosummary does not use the given template

I'm not sure if this is a bug at all, it's just that autosummary hands the template over to autodoc but does nothing else with it, and I'd like to instead have it parse the template and decide the documenter from that.

your documenters do not implement can_document_member() method

They don't, the difference between normal methods and the ones I'm trying to document is not big enough to be able to use that function. I'm only able to reliably detect them using the template option.

What is it different from Documenter.can_document_member()?

The data it has access to, I guess? With can_document_member we get member, membername, isattr and parent while the proposed get_documenter function would get real_name and the Autosummary class in addition to obj and parent.

@keewis
Copy link
Contributor Author

keewis commented Aug 1, 2020

gentle ping, @tk0miya. If you agree with the general approach (allowing to register a custom get_documenter function), I can try to write a draft PR that implements that.

@tk0miya
Copy link
Member

tk0miya commented Aug 1, 2020

Yes, please. I can't answer you well because I'm confusing. A draft PR might help me.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extensions:autosummary type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants