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

always use separate fields for param and type in ext.autodoc.typehints #7562

Merged
merged 2 commits into from May 2, 2020

Conversation

afflux
Copy link
Contributor

@afflux afflux commented Apr 26, 2020

this fixes issues where annotations such as x: typing.Tuple[int, int]
will be transformed to :field typing.Tuple[int, int] x:, which renders
as *int] x* (**typing.Tuple[int,**) --

Feature or Bugfix

  • Bugfix

Detail

Minimum example:

mkdir -p sphinx_signature_type_hints
cd sphinx_signature_type_hints

cat <<'EOF' >type_hint_test.py
import typing
def test(x: typing.Tuple[int, int] = ()) -> int:
    return 1
EOF

mkdir -p docs

cat <<'EOF' >docs/conf.py
extensions = ["sphinx.ext.autodoc", "sphinx.ext.autodoc.typehints"]
autodoc_typehints = 'description'
EOF

cat <<'EOF' >docs/index.rst
.. automodule:: type_hint_test
    :members:
    :undoc-members:
EOF

mkdir -p html
python3.8 -m sphinx -vvv -W -b html --keep-going docs html

Unfortunately, I didn't find a testsuite for sphinx.ext.autodoc.typehints, so I can't promise that this doesn't have any weird side effects.

this fixes issues where annotations such as `x: typing.Tuple[int, int]`
will be transformed to `:field typing.Tuple[int, int] x:`, which renders
as `*int] x* (**typing.Tuple[int,**) -- `
@eric-wieser
Copy link
Contributor

eric-wieser commented Apr 27, 2020

Tests probably should be added to this one:

@pytest.mark.sphinx('text', testroot='ext-autodoc',
confoverrides={'autodoc_typehints': "description"})
def test_autodoc_typehints_description(app):
app.build()
context = (app.outdir / 'index.txt').read_text()
assert ('target.typehints.incr(a, b=1)\n'
'\n'
' Parameters:\n'
' * **a** (*int*) --\n'
'\n'
' * **b** (*int*) --\n'
'\n'
' Return type:\n'
' int\n'
in context)

which pulls signatures from this file

def incr(self, a: int, b: int = 1) -> int:
return a + b

It might make sense to rewrite the test in the form of test_autodoc_typehints_none, which checks the generated RST rather than the final output.

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!
I'll merge this after updates of testcase. Please follow @eric-wieser 's advice :-)

@afflux
Copy link
Contributor Author

afflux commented Apr 30, 2020

It might make sense to rewrite the test in the form of test_autodoc_typehints_none, which checks the generated RST rather than the final output.

Somehow, when I set up the test like this, I don't get any typehints in the output at all. Why is that?

@pytest.mark.sphinx('text', testroot='ext-autodoc',
                    confoverrides={'autodoc_typehints': "description"})
def test_autodoc_typehints_description(app):
    options = {"members": None,
               "undoc-members": True}
    actual = do_autodoc(app, 'module', 'target.typehints', options)
    assert list(actual) == ...

yields:

[...]
.. py:function:: decr(a, b=1)
   :module: target.typehints
.. py:function:: incr(a, b=1)
   :module: target.typehints
[...]

So the built RST looks identical to autodoc_typehints=None.

@eric-wieser
Copy link
Contributor

That should be 'html' not 'text', I assume?

@afflux
Copy link
Contributor Author

afflux commented Apr 30, 2020

No difference with 'html'.

Typehints are added on object-description-transform, which is never reached because apparently do_autodoc only does some basic generation. I'm clearly not very familiar with the output generation stages and I think it might be a good idea for you to fix the tests yourself to save both me and you some headaches.

@eric-wieser
Copy link
Contributor

eric-wieser commented Apr 30, 2020

Huh, that's a pain, but explains why that test is written differently.

I don't know how to proceed here, @tk0miya will have to advise what to do next.

@tk0miya
Copy link
Member

tk0miya commented May 1, 2020

Yes, do_autodoc() is not good for this case as you said. Please use app.build() instead. I think it would be better to eric's idea.

  1. Add a new function to https://github.com/sphinx-doc/sphinx/blob/3.x/tests/roots/test-ext-autodoc/target/typehints.py
    We already have a function having a bit complex type. So new one is not needed in this case.

    def complex_func(arg1, arg2, arg3=None, *args, **kwargs):
    # type: (str, List[int], Tuple[int, Union[str, Unknown]], *str, **str) -> None
    pass

  2. Add an autofunction entry to https://github.com/sphinx-doc/sphinx/blob/3.x/tests/roots/test-ext-autodoc/index.rst
    autofunction:: target.typehints.incr is a good example for this. The function is also declared at target/typehints.py.

  3. Update the case test_autodoc_typehints_description(). It tries to build tests/roots/test-ext-autodoc and check the generated index.txt.

@afflux
Copy link
Contributor Author

afflux commented May 1, 2020

test_ext_autodoc.test_autodoc() fails with the suggested change. Please let me know if you want me to:

a. add the warning to the expected warnings in test_ext_autodoc.test_autodoc()
b. get rid of the warning by replacing the Unknown type in the test case
c. get rid of the warning by creating a new function without the Unknown type and use that for the test_autodoc_typehints_description() test case.

@tk0miya
Copy link
Member

tk0miya commented May 1, 2020

Oh, sorry. Let's choose the c. It's better not to share an example between testcases. The last comment is my wrong.

@afflux afflux force-pushed the fix-typehints-with-space branch from c43fcfc to 4712980 Compare May 2, 2020 06:59
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!

@tk0miya tk0miya merged commit 257550a into sphinx-doc:master May 2, 2020
@tk0miya
Copy link
Member

tk0miya commented May 2, 2020

Thank you for your contribution!

tk0miya added a commit that referenced this pull request May 2, 2020
@tk0miya
Copy link
Member

tk0miya commented May 10, 2020

Note: manually merged into 3.x branch

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants