-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Initial support for PEP 695 type aliases #13508
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
Conversation
89caa94 to
ef7dbce
Compare
|
Thanks Martin! Please could you resolve conflicts? cc also @picnixz if you want to have a look. A |
|
done |
picnixz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised by how easy it was to add this support...
That's probably because of how we parse the signature. This is a part I wrote so I could try to help you later this week (I'm sorry I got disconnected quite a lot with Sphinx now that I'm contributing directly to CPython).
Again that's me who wrote that part for method and the rest. We didn't have the type directive yet and I think I forgot to update this. There's an open issue somewhere where I said I'll take care of it but I forgot. My bad.
I'm surprised: the parser actually supports this. But cross-referencing may not be entirely supported though. |
042c7ec to
74e759d
Compare
Signed-off-by: Martin Matous <[email protected]>
|
Updated with requested changes.
The links would be nice. I'd like to incorporate them in this PR provided the necessary changes are small enough. If it's more work I'll still make them happen, but I wouldn't want this PR to be bogged down because of it. Just point me in the right direction for now. I'll figure it out. |
sphinx/ext/autodoc/__init__.py
Outdated
| ): | ||
| self.add_line(' :canonical: %s' % canonical_fullname, sourcename) | ||
|
|
||
| if isinstance(self.object, TypeAliasType): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typing_extensions.TypeAliasType is a different object than typing.TypeAliasType (even on 3.12 where both exist).
See litestar-org/litestar#3982 (comment) for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Doesn't seem relevant in this context, though. Only one of them will be imported at any given time (0912a5e#diff-e43bdd6f8f37a12d2536e09e57c5e8999cb8de18b9c7ba49126f90576c4328acR57), so the parsed code will always be interpreted consistently as either one or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way for the above if to be executed? also, not isinstance(self.object, NewType) is always true so we should remove it. I just don't want to add 2 canonical lines. So we should first check for TypeAliasType first maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Python >= 3.12, it should match both typing.TypeAliasType and typing_extensions.TypeAliasType. Someone might explicitly create a typing_extensions.TypeAliasType object in order to create a type alias while still supporting Python < 3.12.
picnixz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a lot of time, but here's another round of review.
CHANGES.rst
Outdated
| * #13704: autodoc: Detect :py:func:`typing_extensions.overload <typing.overload>` | ||
| and :py:func:`~typing.final` decorators. | ||
| Patch by Spencer Brown. | ||
| * #13508: Initial support for PEP 695 type aliases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * #13508: Initial support for PEP 695 type aliases. | |
| * #13508: Initial support for :pep:`695` type aliases. |
sphinx/ext/autodoc/__init__.py
Outdated
| ): | ||
| self.add_line(' :canonical: %s' % canonical_fullname, sourcename) | ||
|
|
||
| if isinstance(self.object, TypeAliasType): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way for the above if to be executed? also, not isinstance(self.object, NewType) is always true so we should remove it. I just don't want to add 2 canonical lines. So we should first check for TypeAliasType first maybe.
sphinx/pycode/parser.py
Outdated
| def collect_doc_comment( | ||
| self, | ||
| # exists for >= 3.12, irrelevant for runtime | ||
| node: ast.Assign | ast.TypeAlias, # type: ignore[name-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like this suppression so how about having an "AssignmentLike = ast.Assign" for < 3.12 and AssignmentLike = ast.Assign | ast.TypeAlias for >= 3.12 and use such annotation?
| # check comments before assignment | ||
| if indent_re.match(current_line[: node.col_offset]): | ||
| comment_lines = [] | ||
| for i in range(node.lineno - 1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of going from 0 to node.lineno - 2 and compute node.lineno - 1 - i, why not using a reversed range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code is just being moved, not changed.
sphinx/pycode/parser.py
Outdated
| if (sys.version_info[:2] >= (3, 12)) and isinstance( | ||
| self.previous, ast.TypeAlias | ||
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (sys.version_info[:2] >= (3, 12)) and isinstance( | |
| self.previous, ast.TypeAlias | |
| ): | |
| elif (sys.version_info[:2] >= (3, 12)) and isinstance( | |
| self.previous, ast.TypeAlias | |
| ): |
|
Note: The issue from #10785 also applies to The easiest solution would be to add the Alternatively, the Or we could introduce yet another role like |
|
@mmatous It would be great to move this forward --- are you still interested in working on this? I started my own monkey-patching implementation of this in the sphinx-immaterial theme here: jbms/sphinx-immaterial#467 A few differences are that I have also mapped PEP 613 The |
|
@jbms would you have time to open a rebased PR? A |
Yeah I'm actually working on rebasing and addressing other comments now. |
For Python 3.12 and 3.13, both exist and are not the same type.
|
I believe I've now addressed all of the review comments and outstanding issues. Type parameters are still not handled, but that is largely orthogonal to PEP 695 type alias support. |
|
There is still one issue remaining with how type alias objects are stringified in stringify_annotation that I need to look into. |
I think it is okay as is actually. The issue is that you can define a type alias as a class member, e.g.: class Foo:
type X = intHowever, the resultant In principle, inside of However, the same issue also applies to It might be nice to add a note about these caveats to the autodoc documentation, though. |
|
@AA-Turner I think this is ready now. |
|
Thanks Jeremy, I've pushed some final adjustments. One last question from me -- should we include A |
|
I had intended that explicitly created As far as using The main reason to keep it as
Reasons to keep it as a separate directive:
If we add an Overall it seems like making a separate |
@jbms they still are, no? I thought that there was a test for this. |
I've now split type aliases into a new directive, which I prefer. You make a good point on directive options. A |
AA-Turner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll get this in now so that it doesn't linger further. Thanks @mmatous for opening the initial draft and to @jbms for helping get it accross the line!
Jeremy -- if my changes accidentally broke typing_extensions.TypeAliasType on 3.11 & earlier, sorry -- please could you open a new issue/PR if this is the case?
A
|
@AA-Turner thanks! |
Purpose
Hi, this PR includes support for documenting PEP 695 type aliases. The main problem was that while looking for docstrings, Sphinx's parser didn't recognize ast.TypeAlias as visitable (for doc comment, missing
visit_TypeAlias()) and TypeAliasType object wasn't recognized as something documentable when encountering docstring invisit_Expr()(for docstrings).The rest of it is mostly just me guessing how stuff should be rendered into ReST and trying to ensure Python 3.11 compatibility.
I put the relevant autodoc parts into
ClassDocumentersince that's where code for other type alias variants lives.Couple of caveats:
Type aliases in signatures don't get cross-linked in HTML. I could use some pointers here. I think the ReST is mostly fine and the problem lies in HTML gen.? What should I do about that?
Generic type aliases do not get rendered as such. E.g.
type A[T] = list[T]yields onlytype A = list[T]in resulting HTML. This seems expected, since there is no supported syntax for type params inpy:type, unlikepy:methodDoes not include support for PEP 695 type parameters in generic classes (
class ClassA[T: str]:) or functions/methods (def func[T](a: T, b: T) -> T:). I thought about it briefly and it seems more complicated than I'm willing to tackle rn.How should I go about ruff accepting the test file? Add exemption to pyproject? SyntaxError bc of targeting Py3.11 can't be suppressed.
References