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

Add type hints according to PEP 484 and PEP 604 #1736

Merged
merged 34 commits into from
Mar 22, 2022
Merged

Add type hints according to PEP 484 and PEP 604 #1736

merged 34 commits into from
Mar 22, 2022

Conversation

TonyCrane
Copy link
Collaborator

@TonyCrane TonyCrane commented Feb 12, 2022

Add type hints according to PEP 484, PEP 526 and PEP 604

Motivation

The motivation for this is to allow the IDE (language server) to better analyze the code and give type hints like:
image
And this also provides convenience for writing API documentation in the next step

Changes

I manually wrote type annotations for almost all class methods and functions following these PEP standards.
However, due to the dynamic addition of attributes using CONFIG and digest_config, the types of some attribute values ​​cannot be annotated and therefore the whole module cannot be checked with static type checkers such as mypy. So this annotation is exactly like a comment.

And it's worth mentioning that in this pull request, I upgraded the python version available for ManimGL to python 3.7 and above (i.e. python 3.6 is no longer supported). Because the type annotation system has been relatively complete since 3.7, and numpy no longer supports 3.6 since 1.20 (the numpy.typing submodule has only been added since version 1.20)
I'm not sure if it's worth giving up on 3.6, but maybe that's a general trend.

Because the changes in this PR are almost type annotations, except for some additions of import, it will not affect the normal usage (python will not check whether the variable type satisfies the annotation at runtime)

By the way, in this pull request, I also changed the order of imports according to the PEP 8 (that is, from __future__ import first, then an empty line for built-in modules, an empty line for third-party modules, and an empty line for local modules)

Progress

  • manimlib.utils
  • manimlib.camera
  • manimlib.mobject
    • manimlib.mobject.mobject
    • manimlib.mobject.types
    • manimlib.mobject.svg
  • manimlib.animation
  • manimlib.event_handler
  • manimlib.scene
  • manimlib.shader_wrapper
  • manimlib.window

@TonyCrane TonyCrane marked this pull request as ready for review February 15, 2022 12:55
@TonyCrane TonyCrane marked this pull request as draft February 15, 2022 13:05
@TonyCrane
Copy link
Collaborator Author

Wait for #1745

@TonyCrane
Copy link
Collaborator Author

I think this pull request needs more time to check, because it changes too much.
So any thoughts now? @3b1b

@3b1b
Copy link
Owner

3b1b commented Feb 15, 2022

This is definitely a welcome push in the right direction. I can block out some time a bit later this week to give it a more thorough look.

You mention issues associated with the current "digest_config" usage. At some point, it would be worth getting rid of that practice entirely, and to instead put all the variables current in a CONFIG dict into the init args, like normal python programmers :).

@YishiMichael
Copy link
Contributor

I'd recommend that methods implemented in base classes and then overridden should keep input and output types maintained. As an example, some methods concerned with Mobject don't return self, while they do in an inherited class, which should be unified.

@TonyCrane TonyCrane marked this pull request as ready for review February 16, 2022 13:10
@TonyCrane TonyCrane requested a review from 3b1b February 16, 2022 13:10
@TonyCrane TonyCrane changed the title Add type hints according to PEP 484 and PEP 604 [WIP] Add type hints according to PEP 484 and PEP 604 Feb 16, 2022
@3b1b 3b1b merged commit 0c8b333 into 3b1b:master Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants