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

Fix 10605 constructor annotations in description #10880

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

robsdedude
Copy link

@robsdedude robsdedude commented Sep 28, 2022

@@ -2345,7 +2345,11 @@ def test_autodoc(app, status, warning):
my_name

alias of Foo"""
assert warning.getvalue() == ''
warning_lines = [line for line in warning.getvalue().split('\n') if line]
# This might be a bug. We're not picking up the TypeVar T.
Copy link
Author

Choose a reason for hiding this comment

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

To clarify: the test file tests/roots/test-ext-autodoc/target/typehints_lazy.py causes this warning with or without my changes. Not sure if I did something wrong in the test example or if this needs fixing in autodoc.

robsdedude and others added 2 commits September 28, 2022 12:43
Constructor annotations where not properly detected in autodoc when
`autodoc_typehints` was `'description'` or `'both'`.
 * added missed event emission
 * fixed tests
 * extended tests to include type comments
@robsdedude robsdedude force-pushed the fix-10605-constructor-annotations-in-description branch from 23516fc to 862db92 Compare September 28, 2022 10:44
@robsdedude
Copy link
Author

Potentially pending: updating the docs with the new event. But I'd first like to get feedback if it's even worth putting further effort into this approach.

@AA-Turner AA-Turner added this to the 5.3.0 milestone Sep 29, 2022
@AA-Turner
Copy link
Member

By constructor annotations, do you mean only __init__, or something else?

I'm wary about adding a new event to autodoc if this can be solved another way, I suppose.

Please may you also be a little more specific in your tests--why is LazyGeneric important? Why does the laziness matter?

A

@AA-Turner
Copy link
Member

Please also don't force-push if possible, I squash-merge PRs and GitHub makes reviewing force-pushes much harder than the merge workflow.

A

Lazy annotations (`from __future__ import annotations`) were only introduced in
Python 3.7+.
@robsdedude
Copy link
Author

robsdedude commented Oct 3, 2022

Thanks for having a look.

Please also don't force-push if possible.

Sorry. I only do this before the review process has begun, as I know how hard it is to figure out what changed between reviews when force pushed.

By constructor annotations, do you mean only __init__, or something else?

__init__, yes. And maybe __new__, I'm unsure. I'm talking about this feature where autodoc combines the docstring of the class with the one of its constructor.

I'm wary about adding a new event to autodoc if this can be solved another way, I suppose.

I'm not sure this is possible without a big rewrite. Right now, iirc (this work is a few weeks old already), the code that puts together the types for the combined documentation is a processor (sphinx/ext/autodoc/typehints.py) that only post-processes the documentation nodes. And there is no event (at least that I could find) that correctly emits the (resolved) type hints from the analysis stage (sphinx/ext/autodoc/__init__.py). If you have an idea for an alternative approach, please share.

Please may you also be a little more specific in your tests--why is LazyGeneric important? Why does the laziness matter?

Because inspect.signature only gives you unresolved strings instead of the actual classes/types when annotations are lazily evaluated (iirc, default since Python 3.10).

@robsdedude
Copy link
Author

I'm also battling the tests right now. The issue is that from __future__ import annotations was only introduced in Python3.7. So I don't see a clean way of only rendering the target.typehints_lazy module when Python >= 3.7 I've tried putting it in its own file together with

if sys.version_info < (3, 7):
    exclude_patterns = ['index_py37.rst']

But then I'm still stuck with index_py37.rst: WARNING: document isn't included in any toctree. I sure could expect that warning in the tests, that's what I ended up doing for now, but I'm really not fond of the solution.

@AA-Turner
Copy link
Member

I am delaying this to 6.0 or 6.1, which will be the next releases after 5.3. It will also solve the test issues as 3.6/3.7 support has been dropped.

A

@AA-Turner AA-Turner modified the milestones: 5.3.0, 6.x Oct 15, 2022
@AA-Turner AA-Turner changed the base branch from 5.x to master October 15, 2022 21:17
@robsdedude
Copy link
Author

@AA-Turner 6.1 is out already and this fix sadly still didn't land. It'd really be a great deal if we could merge this or find a different solution. Unresolved type aliases reduce the usability of the docs significantly.

@AA-Turner AA-Turner modified the milestones: 6.x, 7.x Apr 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants