Skip to content

Conversation

henrikmidtiby
Copy link
Contributor

@henrikmidtiby henrikmidtiby commented Apr 27, 2025

Overview: What does this pull request change?

More work on #3375

Continuation of PR #4134

The module manim/_config/utils.py is now going through mypy checks without raising any issues.

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

@henrikmidtiby
Copy link
Contributor Author

@JasonGrace2282 Would you take a look at this PR?

@henrikmidtiby henrikmidtiby force-pushed the typing-logger-utils branch from 5484d9e to 3dd725d Compare May 23, 2025 07:04
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.

Seems fine to me, thank you very much!

@github-project-automation github-project-automation bot moved this from 🆕 New to 👍 To be merged in Dev Board Aug 11, 2025
@behackl behackl enabled auto-merge (squash) August 11, 2025 17:14
@behackl behackl added this to the v0.19.1 milestone Aug 11, 2025
@behackl behackl added the typehints For adding/discussing typehints label Aug 11, 2025
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.

Upon closer inspection (and actually due to our pre-commit mypy check): there now is an issue with the config.window_size key typing that revealed a bug:

The OpenGLWindow does expect config.window_size to be a string of shape 'width,height', or 'default'. If one sets config.window_size = (900, 600) or something like that (which is actually quite reasonable to assume, that an integer-tuple is a suitable choice), then the window crashes because it tries to call .split on the config value.

Suggestion (up for discussion of course): we could rewrite the setter of config.window_size to be tuple[int, int] | None, with None taking over the current role of 'default', and then simplify the logic in opengl_renderer_window.py. Thoughts?

@github-project-automation github-project-automation bot moved this from 👍 To be merged to 👀 In review in Dev Board Aug 11, 2025
@chopan050
Copy link
Contributor

Since we merged PR #4363 which typed opengl_renderer_window.py and stopped ignoring its errors, there was a new error raising in there because of the type of the window_size parameter of Window. I made a commit which fixes that error by allowing window_size to be a tuple of ints and further checking its values.

@chopan050 chopan050 changed the title Typing logger utils - continuation of PR #4134 Add type annotations to manim/_config/utils.py Aug 11, 2025
@chopan050
Copy link
Contributor

chopan050 commented Aug 11, 2025

I just saw @behackl comment. Before reading it, I already committed a fix 😅

Suggestion (up for discussion of course): we could rewrite the setter of config.window_size to be tuple[int, int] | None, with None taking over the current role of 'default', and then simplify the logic in opengl_renderer_window.py. Thoughts?

Would that also make the getter of config.window_size return tuple[int, int] | None? If so, that would be a nice change. I like the idea.

Actually, instead of having too much logic for validating and parsing window_size in Window, there should probably also be a parsing logic inside config.window_size's setter.

EDIT: this should all probably be done in a follow-up PR

@henrikmidtiby henrikmidtiby mentioned this pull request Aug 13, 2025
22 tasks
auto-merge was automatically disabled August 16, 2025 15:01

Head branch was pushed to by a user without write access

@henrikmidtiby
Copy link
Contributor Author

I would suggest to move all the interpretation of the windows_size value into the config file and then skip parsing these values in the constructor for the Window class in the opengl_renderer_window.py file.
I can try to do that in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typehints For adding/discussing typehints
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

4 participants