Skip to content

Conversation

godalming123
Copy link

@godalming123 godalming123 commented Apr 25, 2025

Overview: What does this pull request change?

Number of mypy type checking errors with error ignoring disabled for every file:

  • Before this PR: 1260
  • After this PR: 1249

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

@henrikmidtiby
Copy link
Contributor

This pull request contains several improvements. This can make it harder to get an overview of what is actually changed for each contribution. I have noted the following contributions.

Related to type annotations:

  • Activated type checking for mobject.geometry.* in mypy.ini
  • Updated type annotations in utils/space_ops.py
  • Updated type annotations in utils/images.py
  • Updated type annotations and cleaned up code in utils/bezier.py
  • Updated type annotations in mobject/mobject.py
  • Updated type annotations in graphing/scale.py
  • Updated type annotations in mobject/types/vectorized_mobject.py
  • Updated type annotations in geometry/polygram.py

General code cleanup:

  • Code cleanup in geometry/boolean_ops.py
  • Code cleanup in geometry/arc.py

Potential breaking changes:

  • Replaced keyword arguments with named arguments in mobject/mobject.py in multiple locations. To me this makes it easier to read the code, but I wonder if this is a breaking change to the code?
  • Replaced keyword arguments with named arguments in mobject/types/vectorized_mobject.py

All changes related to type annotations and the general code cleanup looks good to me.

Regarding the potential breaking changes I hope that one of the other maintainers will take a look at that.

@henrikmidtiby henrikmidtiby mentioned this pull request Aug 13, 2025
22 tasks
Copy link
Contributor

@chopan050 chopan050 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 these fixes and changes!

Also thanks to @henrikmidtiby for the overview! By the way, currently this PR does not generate breaking changes when being more explicit with the keyword arguments, because all those kwargs are passed to Mobject.apply_points_function_about_point(), which only accepts some kwargs. If you do, for example Mobject.rotate(nonexistent=True), it will fail with an error, because nonexistent is not a parameter of Mobject.apply_points_function_about_point().

I left some change requests around the code, but almost all of them are related to solving merge conflicts:

@github-project-automation github-project-automation bot moved this from 🆕 New to 👀 In review in Dev Board Aug 13, 2025
Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

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

There are two more suggestions I forgot to include regarding images.py:

Fix type errors, add types for the transformation functions, and fix one case where the about_point and about_edge options were not correctly passed in apply_complex_function in mobject.py
@godalming123 godalming123 requested a review from chopan050 August 14, 2025 07:49
Comment on lines +1230 to +1236
def scale(
self,
scale_factor: float,
*,
about_point: Point3DLike | None = None,
about_edge: Vector3DLike | None = None,
) -> Self:

Check notice

Code scanning / CodeQL

Mismatch between signature and use of an overridden method Note

Overridden method signature does not match
call
, where it is passed too many arguments. Overriding method
method VMobject.scale
matches the call.
Overridden method signature does not match
call
, where it is passed too many arguments. Overriding method
method VMobject.scale
matches the call.
Overridden method signature does not match
call
, where it is passed too many arguments. Overriding method
method VMobject.scale
matches the call.
Overridden method signature does not match
call
, where it is passed too many arguments. Overriding method
method VMobject.scale
matches the call.
Overridden method signature does not match
call
, where it is passed too many arguments. Overriding method
method VMobject.scale
matches the call.
godalming123 and others added 2 commits August 14, 2025 09:58
- Import a couple types that should have been imported
- Remove the redefinition of self.background_stroke_width in manim/mobject/types/vectorized_mobject.py
@godalming123 godalming123 changed the title Fix type errors, enable type checking for mobject/geometry and add typings for the transformation functions Fix type errors and add typings for the transformation functions Aug 14, 2025
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.

3 participants