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

autodoc: Respect type annotations in __new__, metacls.__call__, and builtin types #7384

Merged
merged 2 commits into from May 29, 2020

Conversation

eric-wieser
Copy link
Contributor

@eric-wieser eric-wieser commented Mar 26, 2020

Feature or Bugfix

  • Bugfix

Purpose

None of the following work correctly before this patch

  • Signatures defined by __new__
  • Signatures defined by metaclasses
  • Signatures defined by builtin base classes

Patch includes a test showing that these now all work.

It's possible other tests will have been affected, will update if needed.

Ping @tk0miya, since you recently looked at annotation stuff.

@eric-wieser eric-wieser changed the title Add extended tests for type annotations. Respect type annotations in __new__, metacls.__call__, and builtin types Mar 26, 2020
@eric-wieser
Copy link
Contributor Author

eric-wieser commented Mar 26, 2020

Failures are all of the form

E         -  '.. py:class:: DocstringSig()',
E         ?                             --
E         +  '.. py:class:: DocstringSig',

I think this change is correct - with it, these two classes are now documented identically:

class WithInit:
    def __init__(self):
        pass
class WithoutInit:
    pass

Whereas previously the former would have () but the latter would not.

The two classes give exactly the same result from inspect.signature, so I would consider this a bug.
I've added a second commit to fix this.

@eric-wieser eric-wieser force-pushed the property-annotation branch 2 times, most recently from fa12fb0 to b64a955 Compare March 26, 2020 17:38
@eric-wieser eric-wieser changed the title Respect type annotations in __new__, metacls.__call__, and builtin types autodoc: Respect type annotations in __new__, metacls.__call__, and builtin types Mar 26, 2020
sphinx/ext/autodoc/__init__.py Outdated Show resolved Hide resolved
sphinx/ext/autodoc/__init__.py Show resolved Hide resolved
sphinx/ext/autodoc/__init__.py Outdated Show resolved Hide resolved
sphinx/ext/autodoc/__init__.py Outdated Show resolved Hide resolved
sphinx/ext/autodoc/__init__.py Outdated Show resolved Hide resolved
sphinx/ext/autodoc/__init__.py Outdated Show resolved Hide resolved
tests/test_ext_autodoc_configs.py Outdated Show resolved Hide resolved
tests/test_autodoc.py Outdated Show resolved Hide resolved
@tk0miya
Copy link
Member

tk0miya commented Mar 28, 2020

Should we also modify get_doc() to obtain docstring in order of this sequence?

@eric-wieser
Copy link
Contributor Author

Should we also modify get_doc() to obtain docstring in order of this sequence?

We could, but I don't think that matters so much, and would prefer not to do it in this PR.

@eric-wieser
Copy link
Contributor Author

Comments addressed I think

tests/test_util_inspect.py Show resolved Hide resolved
sphinx/ext/autodoc/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

Sorry for late. Let's go forward this.

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM

sphinx/ext/autodoc/__init__.py Show resolved Hide resolved
@eric-wieser
Copy link
Contributor Author

Needs a rebase to pass tests it seems

@eric-wieser eric-wieser changed the base branch from 3.0.x to 3.x May 3, 2020 11:18
@eric-wieser eric-wieser force-pushed the property-annotation branch 2 times, most recently from 529ef19 to 50c3f3e Compare May 3, 2020 11:35
@eric-wieser
Copy link
Contributor Author

Rebased, hopefully CI will give me some hints this time.

@eric-wieser eric-wieser force-pushed the property-annotation branch 2 times, most recently from 6e67041 to f3a9203 Compare May 7, 2020 10:55
@eric-wieser
Copy link
Contributor Author

CI looks like it will pass now.

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

sphinx/ext/autodoc/__init__.py Outdated Show resolved Hide resolved
sphinx/ext/autodoc/__init__.py Outdated Show resolved Hide resolved
pass

# retry without arguments for old documenters
args = self.format_args()
Copy link
Member

Choose a reason for hiding this comment

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

It seems format_args() is called twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take it back, you're correct. Updated in my latest push.

Comment on lines 394 to 402
try:
args = self.format_args(**kwargs)
except TypeError:
# retry without arguments for old documenters
args = self.format_args()
if kwargs:
try:
args = self.format_args(**kwargs)
except TypeError:
# avoid chaining tracebacks, by putting nothing here
pass

# retry without arguments for old documenters
args = self.format_args()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was called twice before too - this way, we at least:

  • Do not call it twice with identical arguments
  • Do not chain the exception of the first call with the exception of the second

Does anything actually pass non-empty kwargs?

Copy link
Member

Choose a reason for hiding this comment

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

It was called twice before too

It's an unexpected result. The TypeError handler is added only for old Documener classes which does not accept keyword arguments. It implicitly expects format_args() method does not raise TypeError.

In this case, I think no reason to raise TypeError on format_args(). It should be handled inside it.

Does anything actually pass non-empty kwargs?

Yes. The autosummary extension gives keyword argument.

sig = documenter.format_signature(show_annotation=False)

Note: It's after 2 AM in Japan. So this is my last comment today. Good night!

@eric-wieser eric-wieser force-pushed the property-annotation branch 2 times, most recently from 790e6da to b881a28 Compare May 27, 2020 19:38
@eric-wieser
Copy link
Contributor Author

@tk0miya: Think you'll have time to look at this again before 3.1.0?

This fixes:
* Signatures defined by __new__
* Signatures defined by metaclasses
* Signatures defined by builtin base classes

All of these changes bring the sphinx docs inline with the behavior of `inspect.signature`.

Note that this changes autodoc to output `.. py:class: MyClass()` with parentheses even if no user-defined __init__ is present.
This is quite deliberate, as if no user-defined `__init__` is present the default is `object.__init__`, which indeed does not take arguments.
@tk0miya
Copy link
Member

tk0miya commented May 28, 2020

Of course, Yes. But my request is not changed. Please catch a TypeError in format_args(). Then I'll merge this soon.

@eric-wieser
Copy link
Contributor Author

Please catch a TypeError in format_args(). Then I'll merge this soon.

Would you mind adding a comment on the relevant lines? I've moved things around a bit now.

@tk0miya tk0miya added this to the 3.1.0 milestone May 29, 2020
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for your patience and cooperation!

@tk0miya tk0miya merged commit 19ad8a4 into sphinx-doc:3.x May 29, 2020
tk0miya added a commit that referenced this pull request May 29, 2020
@eric-wieser
Copy link
Contributor Author

I assume now #7613 and your todo comment from #3610 will be marginally easier.

@tk0miya
Copy link
Member

tk0miya commented May 29, 2020

Yes, I can improve #7716 now. I'm waiting for merging this :-)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 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.

None yet

2 participants