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(autoaliasattr): search for type aliases under if TYPE_CHECKING #3671

Merged

Conversation

JasonGrace2282
Copy link
Contributor

@JasonGrace2282 JasonGrace2282 commented Apr 1, 2024

Motivation

After #3668 was merged, the module level attributes of manim.mobject.mobject weren't shown
As you can see, this PR fixes that.

Previously, the directive wouldn't find stuff like

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    Updater: TypeAlias = ...

Edge Cases

Please note that these edge cases are unlikely to happen, and are only mentioned for completeness.

TYPE_CHECKING variable

The following will pass the initial filter (it will have to look through the whole body instead of skipping the If node)

TYPE_CHECKING=True
if TYPE_CHECKING:
    ...

This is just a performance thing and not something that can be easily resolved (it's perfectly valid from a Syntax Tree's point of view)

typing.TYPE_CHECKING

The second is this case. I opted to be more specific and only limit it to typing.TYPE_CHECKING. If
there is a consensus to allow the following, it's a one line fix.

import typing as python_typing

if python_typing.TYPE_CHECKING:
    Point2or3D: TypeAlias = Point2D | Point3D

The same edge case as above applies, that is that the following will also have to go through the whole If node:

typing = type('classname', (object,), {'TYPE_CHECKING': True})()
if typing.TYPE_CHECKING:
    ...

@JasonGrace2282 JasonGrace2282 added pr:bugfix Bug fix for use in PRs solving a specific issue:bug documentation Improvements or additions to documentation labels Apr 1, 2024
@JasonGrace2282 JasonGrace2282 marked this pull request as ready for review April 1, 2024 20:17
manim/utils/docbuild/module_parsing.py Dismissed Show dismissed Hide dismissed
@JasonGrace2282 JasonGrace2282 added this to the v0.18.1 milestone Apr 20, 2024
@JasonGrace2282 JasonGrace2282 force-pushed the fix_alias_directive branch 3 times, most recently from 226ddfd to 047ac68 Compare April 20, 2024 21:25
Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

I coarsely read the code changes and mainly looked at the rendered documentation -- this seems fine to me. Thanks!

@behackl behackl enabled auto-merge (squash) April 24, 2024 10:30
@behackl behackl disabled auto-merge April 24, 2024 10:46
@JasonGrace2282 JasonGrace2282 merged commit a4e5233 into ManimCommunity:main Apr 24, 2024
18 checks passed
@JasonGrace2282 JasonGrace2282 deleted the fix_alias_directive branch April 24, 2024 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation pr:bugfix Bug fix for use in PRs solving a specific issue:bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants