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

Make autodoc_typehints="description" work with autoclass_content="class" #8539

Merged

Conversation

godlygeek
Copy link
Contributor

@godlygeek godlygeek commented Dec 15, 2020

Subject: Make autodoc_typehints="description" work with autoclass_content="class"

Feature or Bugfix

  • Bugfix

Purpose

After the fix for #7329, autodoc_typehints="description" does not automatically set the type for class constructor arguments when autoclass_content="class". This fixed a bad behavior: the parameters were being documented twice, once for the class docstring and once for the __init__ docstring.

This PR introduces a better fix for #7329.

Detail

This PR removes the restriction that __init__'s type annotation will not be used to document parameters in the class docstring when autoclass_content="class", and it instead implements a new rule. After this PR, autodoc_typehints="description" will only insert a :type: field if the object's description already contains a :param: for the same parameter, and likewise will only insert an :rtype: field if there is already a :return: field.

Not only does this fix #7329 while still allowing description-mode typehints to work for classes by default, it also fixes several other annoyances with description-mode typehints: undocumented, private parameters like _cache: Dict[str, Any]={} are no longer added automatically, nor are **kwargs added as a single parameter (allowing the user to document the accepted keyword arguments individually, along with their individual types).

I've based the PR on 3.x as I believe this behavior change is a bugfix rather than a breaking change, but I can rebase on master if you prefer.

Relates

@godlygeek godlygeek force-pushed the description_typehints_for_classes branch from 9d055bc to 67737ba Compare December 15, 2020 16:52
@tk0miya
Copy link
Member

tk0miya commented Dec 17, 2020

I feel the requirements of :param: field is a new restriction and incompatible change. So I can't accept this PR as is. But I understand you'd like to suppress type information for private parameters or meaningless kwargs. Do you have idea for this?

@tk0miya tk0miya added this to the 3.5.0 milestone Dec 17, 2020
@godlygeek
Copy link
Contributor Author

godlygeek commented Dec 17, 2020

I feel the requirements of :param: field is a new restriction and incompatible change. So I can't accept this PR as is.

Alright, I can see why you might feel that way. But, if you'll allow me to make the argument, can you see how it could instead be construed as a bug fix, in a feature that is quite new and that has never yet worked correctly?

  1. As things stand today, if you do:
class SomeClass:
    """My Class"""
    def __init__(self, foo: str) -> None:
        ...

then foo is automatically given a type annotation of str in the generated docs, unless you give autodoc_typehints="description", in which case it isn't. Given that this happens only for classes and not for functions, I can't imagine that this is intentional.

  1. As things stand today, if you do:
def some_func(n: int, *, _private_state: int=42):
    """Some func"""

and document it with:

.. automodule:: module

.. autofunction:: some_func(n)

then only the n parameter of foo is documented, and given an annotation of int, unless you give autodoc_typehints="description", in which case a line is automatically added to the docstring for _private_state that contains no information about what it is, beyond that it's an int. This may be the intended behavior, but even so I don't think it's desirable to add lines to the documentation that contain only a parameter's name and type, without any explanation of what the parameter is for. Have you seen anyone using this as a feature, in practice? I can see how :rtype: might be useful even without a :return:, but I honestly can't imagine people intentionally use :type: without :param:, and it seems undesirable for autodoc to generate markup that a user would never choose to write by hand.

I think there's a compelling argument to be made that the current behavior is buggy, and that fixing the bugs is more valuable than maintaining backwards compatibility.

Do we at least agree that neither of these behaviors is what users would want, even if you believe they should be maintained for the sake of backwards compatibility?

@godlygeek
Copy link
Contributor Author

If we do agree that neither of these behaviors is desirable, but you still feel that they must be maintained, perhaps we can add new options that fix these issues, and make those options the default in the next major version?

@tk0miya
Copy link
Member

tk0miya commented Dec 18, 2020

I think it is enough worthy to generate :type: fields even without :param: fields when autodoc_typehitnts = "description". Hence, I don't think this is a bug. On the other hand, I agree with you to control the generation manually (via :param: fields). So +1 for adding a new option. I still don't have an answer to what should be the default behavior at present.

@godlygeek
Copy link
Contributor Author

OK, that seems fair enough, and I wouldn't mind creating an option to control whether or not :type: fields are generated even without :param: fields.

But, then we're back at square one about the one that's definitely a bug. If we generate the :type: fields for class docstrings even when there is no :param: field, then users who use :special-members: __init__ will have have :type: fields added to both the class docstring (where they don't have the :param:) and the __init__ docstring (where they do), which was the bug reported in #7329. The fix for that was to never add :type: fields to the class docstring, which breaks things for everyone who isn't using :special-members: __init__, since now with the default settings :type: fields never get added for __init__ parameters.

If my proposal to only generate :type: fields when there are corresponding :param: fields becomes the default (even if it's able to be turned off with an option), then #7329 is still fixed, and the :type: field will only be added to the place where the parameter is documented, whether it's the class docstring or __init__. But if continuing to generate :type: fields even when there aren't :param: fields remains the default, then we need an alternative fix for #7329.

What do you think?

@tk0miya
Copy link
Member

tk0miya commented Dec 27, 2020

As you said at beginning of this PR, it is a better way to add :param: manually to control where they'd like to extract the type info with the new option. Hence, #7336 should be reverted after the option is added.

@godlygeek
Copy link
Contributor Author

So if I understand you correctly, you would be happy with a solution where a new option is introduced that enables the behavior I propose here (a :type: or :rtype: field is only inserted where a :param: or :return: already exists), and once that option is introduced it's OK to revert #7336 as users will have another workaround available.

If that's the case I can try to find some time to work on this and make that change.

Do you want the option to default to the backwards compatible behavior (:type: and :rtype: inserted unconditionally) and the user needs to opt in to the new behavior (they're only inserted if the parameter or return is documented)?

@tk0miya
Copy link
Member

tk0miya commented Dec 29, 2020

Yes, I prefer that the default behavior is to convert type annotations to info-field-list unconditionally.

@godlygeek
Copy link
Contributor Author

Sorry, this fell of my radar for a while. I've updated the PR to add a new option, autodoc_typehint_undoc, that defaults to True (preserving the current behavior), but that can be set to False to prevent :type: and :rtype: from being added where no :param: or :return: exists.

@@ -548,6 +548,19 @@ There are also config values that you can set:

New option ``'description'`` is added.

.. confval:: autodoc_typehint_undoc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late response. I missed your updates.

Now I'm thinking about the name of this option. I feel this option does not describe its behavior well. If my understanding is correct, it controls the target of the typehints generation to either all of parameters or documented parameters only when autodoc_typehints = "description".

So I propose you the alternatives:

  • autodoc_typehints_description
  • autodoc_typehints_description_target
  • autodoc_typehints_description_params

And it takes "all" or "documented" ("documented_only"?) to change the behavior. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'm fine with that. I've updated the PR to call it autodoc_typehints_description_target and have it take either "all" or "documented".

doc/usage/extensions/autodoc.rst Outdated Show resolved Hide resolved
@godlygeek godlygeek force-pushed the description_typehints_for_classes branch from 79b5f45 to 60afdf7 Compare March 27, 2021 18:36
Previously, if autodoc_typehints="description", a :type: field would be
added for every parameter and return type appearing in the annotation,
including **kwargs and underscore-prefixed parameters that are meant to
be private, as well as None return types.

This commit introduces a new option, "autodoc_typehint_undoc".  By
default this option is True, requesting the old behavior. By setting
this option to False, :type: and :rtype: fields will only be added for
annotated parameters or return types if there is already a corresponding
:param: or :return: field, to put users in control over whether a given
parameter is documented or not.
Previously, `__init__` type hints were not used when documenting a class
using `autodoc_typehints="description"`. This was done to prevent
documentation for parameters from showing up twice, both for the class
and the `__init__` special method. As the new ``autodoc_typehint_undoc``
option provides a better way to prevent this bad behavior by placing the
user in control of where the type hints are added, it is now safe to add
type hints for documented `__init__` parameters.

Closes sphinx-doc#8178
Add new tests to exercise the new autodoc_typehint_undoc option. Where
an existing test would have a meaningful behavior change with the new
option set to False, that test is copied, the new option is set to False
in the copy, and the assertions reflect the new expected behavior.

The new test test_autodoc_typehints_description_with_documented_init
illustrates the problem reported in sphinx-doc#7329, and the new test
test_autodoc_typehints_description_with_documented_init_no_undoc
illustrates that this issue no longer occurs when the new
autodoc_typehint_undoc option is set to False.
@godlygeek godlygeek force-pushed the description_typehints_for_classes branch from 60afdf7 to 4c72848 Compare March 27, 2021 18:54
@godlygeek
Copy link
Contributor Author

And I've now rebased it on master, since you've said you'd like to target 4.0 for it.

@godlygeek godlygeek changed the base branch from 3.x to master March 27, 2021 18:55
@tk0miya tk0miya merged commit ddb6e9c into sphinx-doc:master Apr 3, 2021
@tk0miya
Copy link
Member

tk0miya commented Apr 3, 2021

Merged. Thank you for your great work!

tk0miya added a commit that referenced this pull request Apr 3, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
2 participants