Skip to content

Reduce type: ignore count in _config/utils.py and extract fit-to-size logic from mobject.py#4746

Draft
viniciuspalmieri wants to merge 2 commits into
ManimCommunity:mainfrom
viniciuspalmieri:fix/utils-typing-and-mobject-fit-extraction
Draft

Reduce type: ignore count in _config/utils.py and extract fit-to-size logic from mobject.py#4746
viniciuspalmieri wants to merge 2 commits into
ManimCommunity:mainfrom
viniciuspalmieri:fix/utils-typing-and-mobject-fit-extraction

Conversation

@viniciuspalmieri

Copy link
Copy Markdown

Draft PR addressing the two cleanups discussed in #4734 and #4735. Marking as draft to signal that work is in progress and to invite feedback on the approach before requesting review.

Closes #4734
Closes #4735

Commits

This PR contains two independent commits, one per issue, so either can be reverted without affecting the other.

1. Reduce # type: ignore count in _config/utils.py (#4734)

Brings the file from 10 markers down to 4. The six removed all came from annotations being slightly looser than reality rather than the underlying code being genuinely hard to type:

  • Tightening _d: dict[str, Any | None] to dict[str, Any] removes two [operator] ignores at the frame_y_radius / frame_x_radius getters.
  • Correcting __deepcopy__'s memo annotation from dict[str, Any] to dict[int, Any] removes one [arg-type].
  • Rewriting the a.__setitem__(...) or b.__setitem__(...) chain hack in three setters as two plain assignments removes three [func-returns-value] ignores.

The four remaining markers (Liskov in update(), has-type bootstrap on _tex_template, literal-required on a QUALITIES key, dynamic setattr on a lambda) are structurally harder and left out of scope.

2. Extract fit-to-size logic from mobject.py (#4735)

Moves the implementation of rescale_to_fit and the six *_to_fit_* wrappers out of Mobject and into a new manim/mobject/_fit.py module of free functions. The seven methods remain on Mobject as thin stubs that delegate to the utility functions.

Shape (utility + delegation rather than a mixin) follows the suggestion from #4735 and matches the established pattern in manim/utils/ (space_ops, iterables, etc.). It also avoids the # type: ignore markers a mixin would need, since the utility functions take the mobject as a regular parameter.

The public API is unchanged: Square().scale_to_fit_width(5) works identically. mobject.py's overall LOC count is roughly unchanged (the stubs and docstrings stay), but the substantive implementation now lives in _fit.py (51 LOC). This is meant as a precedent for further cohesive groups (transforms, positioning, color), without claiming a position on a broader split.

Validation

Run locally on Windows 11 / Python 3.12 / uv 0.9.29:

  • ruff check clean on touched files.
  • mypy clean on manim/_config/utils.py and manim/mobject/_fit.py.
  • pytest tests/module/mobject/mobject/ tests/test_config.py: 30 passed, 0 failed (3 tests deselected because they fail identically on unmodified main due to missing latex/dvisvgm/manim CLI on the local machine; verified via git stash).
  • Extended pytest on non-LaTeX-dependent modules: 138 passed.
  • Targeted: pytest -k "scale_to_fit or stretch_to_fit or frame_y_radius or frame_x_radius or frame_size": 3/3 passed.

I haven't run the graphical_units suite (requires ffmpeg); given the nature of the changes (no rendering logic touched), I don't expect issues, but flagging it.

Open questions before moving out of draft

  1. For Reduce # type: ignore count in manim/_config/utils.py #4734: would you prefer this as a single PR for all six fixes (as here), or split into three (one per category)? The categories are small enough that one PR feels right to me, but splitting makes pieces independently revertable.
  2. For Extract fit-to-size methods from mobject.py into a mixin module #4735: confirming the utility + delegation shape is preferred over the mixin shape I initially proposed.
  3. Any objection to the two cleanups landing together, or would you rather have them as separate PRs?

The file currently carries 10 # type: ignore markers. Six of them exist
only because annotations are slightly looser than reality, not because
the underlying code is genuinely hard to type. This commit tightens
those annotations and removes the markers:

- Two [operator] ignores at the frame_y_radius/frame_x_radius getters
  go away by changing _d's annotation from dict[str, Any | None] to
  dict[str, Any] (the | None was what made `_d["frame_height"] / 2`
  illegal; values are still allowed to be None at runtime via Any).

- One [arg-type] ignore in __deepcopy__ goes away by correcting the
  memo parameter annotation: copy.deepcopy's memo uses int keys
  (object ids), not str.

- Three [func-returns-value] ignores in the frame_y_radius,
  frame_x_radius and frame_size setters go away by rewriting the
  `a.__setitem__(...) or b.__setitem__(...)` chain hack as two plain
  assignment statements. Same behaviour, easier to read.

The remaining four markers (Liskov in update(), has-type bootstrap on
_tex_template, literal-required on a QUALITIES key, dynamic setattr on
a lambda) are structurally harder and out of scope here.

mypy manim/_config/utils.py is clean after the change and the existing
tests/test_config.py suite still passes.
… module

mobject.py is by far the largest file in manim/. As a first scoped step
toward reducing its size, this commit moves the implementation of the
fit-to-size family out of the Mobject class body and into a new
manim/mobject/_fit.py module of free functions.

The seven affected methods (rescale_to_fit, scale_to_fit_width,
stretch_to_fit_width, scale_to_fit_height, stretch_to_fit_height,
scale_to_fit_depth, stretch_to_fit_depth) remain defined on Mobject as
thin stubs that delegate to the corresponding functions in _fit. The
public API is unchanged: `Square().scale_to_fit_width(5)` works
identically.

The shape (utility module + delegation, rather than a mixin)
follows the suggestion in issue discussion and matches the established
pattern in manim/utils/ (space_ops, iterables, etc.). It also avoids
the # type: ignore markers a mixin would need, since the utility
functions take the mobject as a regular parameter.

The substantive logic that moves out is the body of rescale_to_fit
(the six wrappers are one-liners). mobject.py's overall LOC count is
roughly unchanged because the method declarations and docstrings stay,
but the actual implementation now lives in _fit.py (51 LOC). This
sets a precedent for further cohesive groups (transforms, positioning,
color) without claiming a position on the broader split.

Validation: ruff clean on the touched files, mypy clean on _fit.py,
pytest passes on tests/module/mobject/mobject/ and tests/test_config.py
(30 passed on the non-LaTeX-dependent subset; the three failures I see
locally on Windows are environmental and fail identically against
upstream main).
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.

Extract fit-to-size methods from mobject.py into a mixin module Reduce # type: ignore count in manim/_config/utils.py

1 participant