Chilipp / docrep

A Python Module for intelligent reuse of docstrings
Apache License 2.0
30 stars 4 forks source link

Docstring inheritance #17

Open Chilipp opened 4 years ago

Chilipp commented 4 years ago

Hey!

There is a new feature that I would like to discuss for docrep and I'd very much like your feedback (pinging @jgostick, @mennthor, @amnona, @lesteve). It's about docstring inheritance (i.e. the docrep.DocstringProcessor inserts parameters based on the base class).

Wouldn't it be nice to have something like

class A:

    def my_method(self, a=1):
        """Do something awesome

        Parameters
        ----------
        a: int
            A number

        Returns
        -------
        int
            The changed number
        """"
        return a + 1

@docstrings.resolve_inheritance
class B:

    @docstrings.inherit_parameters
    @docstring.inherit_returns
    def my_method(self, a=1):
        """Do something better"""
        return a + 2

and then have B.my_method the same docstring as A.my_method? inherit_parameters and inherit_returns would mark the method to be processed then in resolve_inheritance (Note: this separate treatment is necessary as, during class compilation, B is not yet defined and we therefore cannot resolve that B inherits A within inherit_parameters).

I'd like to discuss with you, how this should be set up (besides the implementation discussed above). Here are some ideas that I have and I highly appreciate your feedback and suggestions :smiley:

Specifying the base class

We could extend this by optionally specifying the base class, e.g. via

def funca(a=1):
    """"Do something

    Parameters
    ----------
    a: int
        The input
    """"
    return a+ 1

@docstrings.resolve_inheritance
@docstrings.inherit_parameters(funca)
def funcb(a=1):
    """Do something else"""
    return a + 2

This would then make it available for functions as well.

Replace arguments

We could also replace arguments in the sub class method (see alsp #16), e.g.

def funca(a=1, b=2):
    """"Do something

    Parameters
    ----------
    a: int
        The input (Default 1)
    b: int
        Another input (Default 2).
    """"
    return a+ b

@docstrings.resolve_inheritance
@docstrings.inherit_parameters(funca)
def funcb(a=3, b=2):
    """Do something else

    Parameters
    ----------
    a: int
        The input (Default 3)"""
    return a + 2

Specify parameters

I would in general use inspect.getfullargspec and match the parameters of the function with the parameters of base. But it should also have the possibility to specify them manually, e.g. to resolve stuff like *args, **kwargs. Here is an example

@docstrings.resolve_inheritance
@docstrings.inherit_parameters(funca, params=['a'])
def funcb(*args):
    """Do something else

    Parameters
    ----------
    b: int
        The input (Default 3)"""
    pass

funcb in this case should have used the docstring of parameter a although it is not in the argspec of funcb.

jgostick commented 4 years ago

Firstly, I really like the idea of not having to declare a docstring to be used in a downstream class. This massively simplifies things for developers. If we could just fetch docstrings on demand at the location where they'll be used it would be vastly simpler to use.

Secondly, the idea of inheriting doc strings and adding to them without having to insert the text formatting strings is another major improvement. Again, this is one less thing for devs to worry about. As you've outlined above, the inherit_parameters decorator is super clear in its meaning. I wonder if you really need the resolve_inheritance decorator or if its function is implied by the inherit_parameters decorator?

Finally, as I mentioned in issue #16 I really would like to see a parameter over-write superclass info by default. We have abstract base classes and subclasses where defaults are specified, and I need the subclassed docstring to supersede the parent class.

Overall, the proposed changes look great to me!

jgostick commented 4 years ago

I just changed my mind about point 2 above. I think I would prefer to have the power to insert the specific segments of a docstring in the locations I want. For instance, we have a Settings class in our package, and use docrep to grab the description of settings on the parent classes, and then merge with the new settings on the child. In some cases I am grabbing settings from several class's Parameters sections and lumping them together in the Other Parameters of the child. This also occurs in cases of multiple inheritance.

The optimal api for my particular work flow would be something like:

class A:

    def my_method(self, a=1):
        """Do something awesome

        Parameters
        ----------
        a: int
            A number

        Returns
        -------
        int
            The changed number
        """"
        return a + 1

class B:
    @docstrings.get_super_docs (or @docstrings.get_parent_docs)
    def my_method(self, a=1, b=2):
        """Do something better

        Parameters
        -------------
        %s(A.parameters)s
        b : int (optional, default = 2)
                Another parameter

        Returns
        ---------
        %s(A.returns)s

"""
        return a + 2

In other words, in an ideal world I'd like:

(a) for docrep to grab the docstrings on demand inside the child instead of devs having to specify which parts docrep grabs at the parent class level

and

(b) to be able to pick and choose which components get put in which locations using the current syntax, with the parsing enabled by simply adding a single, clear decorator to the function/method.

Also, as I mentioned in #16, I'd like the child class parameters to overwrite the parent.

Chilipp commented 4 years ago

Hey @jgostick, thanks for your thoughts on this! I think I see your point: You prefer to be a bit more explicit in the docs rather then hiding everything inside the decorator. I like your suggestion, it's intuitive and forces the developer to explicitly tell docrep what should be inserted. It also better integrates with the current functionality.

I like the syntax you propose and with the substitution_pattern that is already implemented (https://github.com/Chilipp/docrep/blob/6ae96d54e1d9ad389559c0aea03aa7e0621aad76/docrep/__init__.py#L19), it would be straight-forward to retrieve the keys and resolve them.

I propose to add three decorators in this case, with reference to the current implemented methods, that all use the params attribute. See the following table

Current Same method but resolves the docstring
DocstringProcessor.__call__ resolve
dedent resolve_and_dedent
with_indent resolve_and_indent

The challenging aspect is, however, to resolve A. Is this a variable in the globals of the decorated function? Or is this something in the MRO of the class? In your example, how should it actually know that it has to use the docstring of A.my_method instead of A.__doc__? I propose to add the functions that should be available as keyword arguments to resolve, etc.. For instance something like


class A:

    def my_method(self, a=1):
        """Do something awesome

        Parameters
        ----------
        a: int
            A number

        Returns
        -------
        int
            The changed number
        """"
        return a + 1

class B:

    @docstrings.resolve_and dedent(A=A.my_method)
    def my_method(self, a=1, b=2):
        """
        Do something better

        Parameters
        ----------
        %(A.parameters)s
        b : int (optional, default = 2)
                Another parameter

        Returns
        -------
        %(A.returns)s
        """"
        return a + 1

It should also be possible to keep or delete certain parameters with this framework. In this case, I would stick to the syntax as it is currently used by keep_params and delete_params, i.e. %(A.parameters.a)s to keep a in the docs, and %(A.parameters.no_a)s to remove it.

Also, as I mentioned in #16, I'd like the child class parameters to overwrite the parent.

with this methodology of course, the check for duplicates would be something on top of the resolve functionality and I would implemented as described in https://github.com/Chilipp/docrep/issues/16#issuecomment-599106305