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 to manim.cli module #3988

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

chopan050
Copy link
Contributor

@chopan050 chopan050 commented Oct 28, 2024

As required by the issue #3375, I added typings for the manim.cli module.

I also added an entry in the docs for the cli module, although it's a work in progress.

Links to added or changed documentation pages

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

@chopan050 chopan050 mentioned this pull request Oct 28, 2024
22 tasks
manim/cli/checkhealth/checks.py Dismissed Show dismissed Hide dismissed
manim/cli/render/global_options.py Fixed Show fixed Hide fixed
manim/cli/render/render_options.py Fixed Show fixed Hide fixed
manim/cli/render/render_options.py Fixed Show fixed Hide fixed
@JasonGrace2282
Copy link
Member

Could you remove this module from mypy's ignore list?

https://github.com/ManimCommunity/manim/blob/main/mypy.ini#L61-L63

@chopan050
Copy link
Contributor Author

Done!

@chopan050
Copy link
Contributor Author

I'll have to fix the MyPy errors later.

manim/cli/default_group.py Fixed Show fixed Hide fixed
manim/cli/render/render_options.py Dismissed Show dismissed Hide dismissed
@chopan050
Copy link
Contributor Author

I fixed all the errors reported by mypy.
In order to do that, I had to modify files outside of manim.cli:

  • manim._config.cli_colors.parse_cli_ctx()'s return value was typed as a cloup.Context, but it was actually a dict[str, Any], so I had to modify it.
  • I had to type OpenGLRenderer.__init__() and Scene.__init__() in order to use them in typed contexts.
  • The manim.utils.file_ops.open_file() function was missing type annotations and I had to use it in a typed context, so I also typed it.

Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

Thanks for putting in this effort, this is amazing progress towards typing Manim fully :)

I did leave some minor comments, so I would appreciate if you could take a look at those.

Comment on lines 25 to 26
if TYPE_CHECKING:
pass
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if TYPE_CHECKING:
pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hold on, how did that even get there-


return isinstance(value, expected) and (is_valid_style(value) if style else True)
return isinstance(value_literal, ExpectedLiteralType) and (
(type(value_literal) is str and is_valid_style(value_literal))
Copy link
Member

Choose a reason for hiding this comment

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

I don't see a strict reason to allow only str and not any subclasses?

Suggested change
(type(value_literal) is str and is_valid_style(value_literal))
(isinstance(value_literal, str) and is_valid_style(value_literal))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think of that case. Thanks for mentioning it!

@@ -245,11 +266,11 @@ def write(level: str = None, openfile: bool = False) -> None:
with cfg_file_path.open("w") as fp:
parser.write(fp)
if openfile:
open_file(cfg_file_path)
open_file(str(cfg_file_path))
Copy link
Member

Choose a reason for hiding this comment

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

This seems to me that open_file should be typehinted to allow manim.typing.StrPath? Or is there a reason why it has to be a string?

Copy link
Member

@JasonGrace2282 JasonGrace2282 Oct 29, 2024

Choose a reason for hiding this comment

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

I just realized that it has to be a pathlib.Path currently, maybe a better approach would be to allow any StrPath and then convert it to a Path manually?

https://github.com/chopan050/manim/blob/8f0af8d4db631a46417d99722e2ad12358b8807b/manim/utils/file_ops.py#L194

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I put str on accident. It should've been pathlib.Path.

Regarding the StrPath, I don't know. There are some inconsistencies in file_ops.py which I think should be handled in a different PR, and I'd prefer to make that change in that PR. There are parameters called file_path, but some of them are of type pathlib.Path and others are str. There are also parameters called file_name whose type is pathlib.Path, as well as a parameter which is simply called file, also of type pathlib.Path.

@@ -269,7 +290,7 @@ def show():
@cfg.command(context_settings=cli_ctx_settings)
@cloup.option("-d", "--directory", default=Path.cwd())
@cloup.pass_context
def export(ctx, directory):
def export(ctx: cloup.Context, directory: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def export(ctx: cloup.Context, directory: str) -> None:
def export(ctx: cloup.Context, directory: manim.typing.StrPath) -> None:

Just allowing it to be less restrictive is a helpful habit :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless we pass type=cloup.Path(path_type=pathlib.Path) to @cloup.option, it doesn't seem that directory can be a pathlib.Path.

description: str
recommendation: str
skip_on_failed: list[str]
post_fail_fix_hook: Callable | None
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind annotating the Callables here and later? If you don't care about the arguments you can do e.g. Callable[..., object]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

func.post_fail_fix_hook = post_fail_fix_hook
HEALTH_CHECKS.append(func)
return func
def wrapper(func: Callable) -> HealthCheckFunction:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def wrapper(func: Callable) -> HealthCheckFunction:
def wrapper(func: Callable[[], bool]) -> HealthCheckFunction:

If I understood correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I forgot that it's only intended to wrap functions like that.

:class:`bool`
Whether ``dvisvgm`` is in ``PATH`` and can be executed or not.
"""
path_to_dvisvgm: str | None = shutil.which("dvisvgm")
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this str | None annotation? shutil.which should take care of it for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also had it for the is_manim...() functions. I removed those, but I missed these for latex and dvisvgm. I'll also remove them.

# No command name matched.
ctx.arg0 = cmd_name
ctx.meta["arg0"] = cmd_name
Copy link
Member

Choose a reason for hiding this comment

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

Didn't know about this, nice change :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I was struggling to do this assignment without having mypy complain about it, so I ended up looking at click.Context in the hope of finding some dictionary, which I fortunately found!

@@ -94,11 +98,12 @@ def update_cfg(cfg_dict: dict, project_cfg_path: Path):
help="Default settings for project creation.",
nargs=1,
)
def project(default_settings, **args):
def project(default_settings: bool, **args: Any) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def project(default_settings: bool, **args: Any) -> None:
def project(default_settings: bool, **kwargs: Any) -> None:

I know I know, not related to the PR per say, but it would be nice if you could do this here and below :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

"""Creates a new project.

PROJECT_NAME is the name of the folder in which the new project will be initialized.
"""
project_name: Path
Copy link
Member

Choose a reason for hiding this comment

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

(nitpick?) Maybe instead of this, it could be something like

project_name = cast(Path, args["project_name"]) or click.prompt("Project Name", type=Path)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I don't know about this. I could add the cast, although mypy didn't complain about this. However, the proposal with or seems a bit more difficult to visually digest, at least for me.

@JasonGrace2282 JasonGrace2282 changed the title Add type hints to cli Add type hints to manim.cli module Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

2 participants