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

register custom autosummary get_documenter functions #8038

Merged
merged 11 commits into from Aug 7, 2020

Conversation

keewis
Copy link
Contributor

@keewis keewis commented Aug 2, 2020

Feature or Bugfix

  • Feature

Purpose

This is a rough draft of the feature I proposed in #7908. Using this to override get_documenter would allow me to rewrite my hack to something like

def get_documenter_from_template(autosummary, obj, parent, real_name):
    ...

and then to register it:

def setup(app):
    app.add_autosummary_get_documenter(get_documenter_from_template)

I don't really need all the information from the Autosummary class (only autosummary.env.app, autosummary.config.autosummary_context (maybe? I don't really understand that value), autosummary.options and real_name), but I thought the hook should be pretty general so it doesn't only solve my own use case. So as long as I have access to that information, I'm happy to change the signature.

Also, if there's any way to solve my problem without overriding get_documenter (I can't use can_document_member and priority because there's no way to decide from obj whether the documenter is appropriate), I won't insist on this feature.

@keewis keewis changed the title register custom get documenter functions register custom autosummary get_documenter functions Aug 2, 2020
keewis added a commit to xarray-contrib/sphinx-autosummary-accessors that referenced this pull request Aug 2, 2020
@keewis keewis marked this pull request as ready for review August 6, 2020 14:54
@keewis
Copy link
Contributor Author

keewis commented Aug 6, 2020

@tk0miya: any chance this can be included in 3.2.0? It would really help me with fixing my extension, but I totally understand if you want to think about this for a bit longer.

@tk0miya
Copy link
Member

tk0miya commented Aug 7, 2020

I can't imagine what extension this helps except yours. I understand your extension need to access the template option of autosummary directive. But I think they should not be related. So I can't allow merging this change. Only I can do is making get_documenter replaceable easily in subclasses for overriding the directive by extensions:

class Autosummary:
    def get_items(...):
        ...
        doccls = self.get_documenter(self.env.app, obj, parent)
        ...

    def get_documenter(self, app, obj, parent):
        return get_documenter(app, obj, parent)  # Extensions can override this method and overide the directive forcedly.

@keewis
Copy link
Contributor Author

keewis commented Aug 7, 2020

that would work, too, but it would make all extensions using this mechanism conflict with each other. Not sure if that's an issue, though.

@keewis
Copy link
Contributor Author

keewis commented Aug 7, 2020

I just noticed that moving get_documenter to Autosummary is not possible because some functions in sphinx.ext.autosummary.generate use it and don't have access to a Autosummary object. How about adding a wrapping method? That way my proposed changes would become just

diff --git a/sphinx/ext/autosummary/__init__.py b/sphinx/ext/autosummary/__init__.py
index c1d0febe0..32d49ca29 100644
--- a/sphinx/ext/autosummary/__init__.py
+++ b/sphinx/ext/autosummary/__init__.py
@@ -281,6 +281,10 @@ class Autosummary(SphinxDirective):
 
         return nodes
 
+    def get_documenter(self, app: Sphinx, obj: Any,
+                       parent: Any, name: str) -> "Type[Documenter]":
+        return get_documenter(app, obj, parent)
+
     def get_items(self, names: List[str]) -> List[Tuple[str, str, str, str]]:
         """Try to import the given names, and return a list of
         ``[(name, signature, summary_string, real_name), ...]``.
@@ -313,7 +317,7 @@ class Autosummary(SphinxDirective):
                 full_name = modname + '::' + full_name[len(modname) + 1:]
             # NB. using full_name here is important, since Documenters
             #     handle module prefixes slightly differently
-            doccls = get_documenter(self.env.app, obj, parent)
+            doccls = self.get_documenter(self.env.app, obj, parent, real_name)
             documenter = doccls(self.bridge, full_name)
             if not documenter.parse_name():
                 logger.warning(__('failed to parse name %s'), real_name,

(I seem to really need real_name for filling in the template)

@tk0miya
Copy link
Member

tk0miya commented Aug 7, 2020

Sorry, I can't understand your proposal. Even if we added real_name to the parameters, sphinx.ext.autosummary.generate is not affected because they are not related to the Autosummary directive object.

The generate_autosummary_* functions surely scan the reST parses and detect autosummary directive calls. But they don't use reST parsers. So no directive object is instantiated on parsing. So the change of Autosummary directive class does not affect the functions at all.

@tk0miya
Copy link
Member

tk0miya commented Aug 7, 2020

I feel strange to add real_name to the get_documenter() method. But I can accept if we change it to create_documenter() method. It's understandable.

@keewis
Copy link
Contributor Author

keewis commented Aug 7, 2020

some functions in sphinx.ext.autosummary.generate call sphinx.ext.autosummary.get_documenter (the function), so if we remove that / convert it to a method on Autosummary, those functions are going to break.

But I can accept if we change it to create_documenter() method

sure, that sounds good.

@tk0miya
Copy link
Member

tk0miya commented Aug 7, 2020

Understand. I didn't mean to remove sphinx.ext.autosummary:get_documenter() function itself. I only proposed you to add a new method Autosummary.get_documenter() as a helper to override it for you (at that time).

@keewis
Copy link
Contributor Author

keewis commented Aug 7, 2020

okay, then that was my bad. I'll rewrite the PR.

@keewis
Copy link
Contributor Author

keewis commented Aug 7, 2020

If the way it is now is acceptable, I think we should discuss the way this is meant to be overridden. While forcibly overriding the Autosummary class used by the autosummary directive is certainly possible, it means that only a single extension using this mechanism can be used. Instead, we could provide a function to register it:

def register_create_documenter(func):
    original_func = Autosummary.create_documenter

    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        result = func(*args, **kwargs)
        if result is not NotImplemented:
            return result
        else:
            return original_func(*args, **kwargs)

    Autosummary.create_documenter = wrapper
    return wrapper

of course, that will still break as soon as someone overrides the autosummary directive using a custom class that does not inherit from Autosummary or Autosummary is converted to use __slots__.

Is this something you would consider including?

@tk0miya
Copy link
Member

tk0miya commented Aug 7, 2020

Is this something you would consider including?

No. It's too much. Let's discuss again after another extension appeared. I can't imagine the extension what needs to hook this. I just made a new method only for you.

@keewis
Copy link
Contributor Author

keewis commented Aug 7, 2020

Let's discuss again after another extension appeared

sure

I just made a new method only for you

thank you very much for that!

keewis added a commit to xarray-contrib/sphinx-autosummary-accessors that referenced this pull request Aug 7, 2020
@tk0miya tk0miya added this to the 3.2.0 milestone Aug 7, 2020
@tk0miya tk0miya merged commit f92fa64 into sphinx-doc:3.x Aug 7, 2020
@tk0miya
Copy link
Member

tk0miya commented Aug 7, 2020

Merged!

@keewis
Copy link
Contributor Author

keewis commented Aug 7, 2020

thanks a lot, @tk0miya. 🎉

@keewis keewis deleted the custom-get_documenter branch August 7, 2020 16:16
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autosummary with custom documenters
2 participants