Fix Typing #3086
Fix Typing #3086
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
MrDiver
added
maintenance
|
Well, then let's mark this as ready for review. It's likely that I'll have time to check this later today. 👍 |
There was a problem hiding this comment.
I've looked through the proposed changes here, this looks mostly very clean -- thanks to everyone who has put time and effort into this, much appreciated!
I'll leave my comments here, my intention at this point is to get this PR merged as soon as possible, substantial further changes should at this point definitely happen in follow-up PRs. (It really is time for a new release rather sooner than later!)
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Thanks again everyone involved here, this is a great starting point for a well-typed library 🚀 |
| Vector: TypeAlias = npt.NDArray[PointDType] | ||
| """ `shape: (N,)` A Vector `[float,...]`. """ | ||
|
|
||
| RowVector: TypeAlias = npt.NDArray[PointDType] |
There was a problem hiding this comment.
For this @alembcke @Viicos i would like to stick to Type aliases because then we can adapt to changes in the future so neither Sequence[float] nor np.ndarray
But rather using the types in our typing.py such that we have -> RowVectoror any of the other types here, because then we can adapt more quickly and test out things, for example as soon as Annotated works in mypy I would switch back to using it here for the array shapes.
| @classmethod | ||
| def parse( | ||
| cls, | ||
| color: ParsableManimColor | list[ParsableManimColor] | None, |
There was a problem hiding this comment.
In this case it should be as concise as possible what happens, i am still not sure on how to go about the lists in list problem here because you can pass a list of rgb Arrays so i would like to have it as explicit as possible that it is either the normal types or a python list because a sequence has more types than this function can accept and return a valid result.
manim/utils/color/manim_colors.py
Outdated
| LIGHT_BROWN: ManimColor = ManimColor("#CD853F") | ||
| DARK_BROWN: ManimColor = ManimColor("#8B4513") | ||
| GRAY_BROWN: ManimColor = ManimColor("#736357") | ||
| GREY_BROWN: ManimColor = ManimColor("#736357") |
pyproject.toml
Outdated
| [tool.poetry.group.dev.dependencies] | ||
| mypy = "^0.991" | ||
| types-tqdm = "^4.64.7.9" | ||
|
|
There was a problem hiding this comment.
I didn't get to moving everything in to the if statements i got a lot of them done by now but if you find places where I didn't put it in there just move them in to the if TYPE_CHECKING.
manim/mobject/mobject.py
Outdated
| return self | ||
|
|
||
| def set_color_by_gradient(self, *colors): | ||
| def set_color_by_gradient(self, *colors: Iterable[ParsableManimColor]): |
There was a problem hiding this comment.
Makes sense, can we add that to the Discussion tab ?
manim/mobject/mobject.py
Outdated
|
|
||
| # Errors | ||
| def throw_error_if_no_points(self) -> None: | ||
| def throw_error_if_no_points(self) -> None | Never: |
There was a problem hiding this comment.
I think it should be fine for now, there is not that much left to do, we already got the Colors out of the windows now and what's left is currently not doable because of recursive imports so the next thing that is merged will be this PR and probably nothing else so we have a clean slate of manim that we can work on for the other PRs which makes a lot more sense then.
|
Late approved, but approved 🏁 thanks for all the help on this PR! ❤️ |
Overview: What does this pull request change?
It has been a long time without types in manim and i want to change that. Feel free to help on this journey.
Very Important Knowledge Base
Link to updated typing guide in the documentation: https://manimce--3086.org.readthedocs.build/en/3086/contributing/typings.html
closes #52