ManimCommunity / ManimCommunity/manim
Reduce # type: ignore count in manim/_config/utils.py
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 40.9k
- Forks
- 3.1k
- Avg merge
- 3d 12h
- Merged PRs (30d)
- 25
Description
While reading manim/_config/utils.py I noticed it carries an unusual number of # type: ignore markers: ten in total, in a 1897-line file. Looking at the recent git history of the file, several of the bug fixes touched the same areas that the ignores are silencing, which made me want to look at each one and ask whether it could be replaced by an honest type annotation.
The reasoning is pretty standard: every # type: ignore is a deferred contract. You're telling the checker "trust me, this is fine," and from that point on any future change in the surrounding code can break the assumption without anyone being warned. That's not always avoidable, but when an ignore exists only because an annotation is slightly looser than reality, fixing the annotation seems strictly better than keeping the marker.
I went through the ten and most of them turn out to be cheap to fix. Six fall into three small groups.
The first group is two [operator] ignores on lines 1145 and 1156 (self._d["frame_height"] / 2 and the equivalent for frame_width). They exist because _d is annotated as dict[str, Any | None], and that | None is what makes the checker reject the division. None / 2 is illegal. The values aren't actually ever None at the point those properties are read; the annotation is just looser than reality. Tightening it to dict[str, Any] makes both ignores go away without changing behaviour (values can still be None at runtime, since Any accepts everything).
The second is a single [arg-type] ignore on line 432, in __deepcopy__. The memo argument is annotated dict[str, Any], but copy.deepcopy's memo dict uses int keys (object IDs). The annotation is just wrong; fixing it to dict[int, Any] resolves the ignore.
The third group is three [func-returns-value] ignores on lines 1149, 1160, and 1293. These come from a pattern the file uses to chain two side-effecting assignments on one line:
self._d.__setitem__("frame_y_radius", value) or self._d.__setitem__("frame_height", 2 * value)
__setitem__ returns None, so the or always evaluates the second call, but the checker (correctly) flags that you're using a None-returning expression as a value. Rewriting each as two plain statements removes the ignore and is also considerably easier to read.
That gets the file from ten ignores down to four. The remaining four are real and harder to deal with: there's a Liskov violation in update() whose signature doesn't match MutableMapping.update, a has-type bootstrap issue with _tex_template, a literal-required from a dict key the checker can't prove is a valid TypedDict literal, and a dynamic setattr on a lambda. Each of those would need its own conversation, so I'd keep them out of scope here.
I have a working patch with all six fixes applied locally; mypy manim/_config/utils.py comes back clean, and the existing tests/test_config.py still passes. Happy to open the PR if there's no objection to the approach. One thing I wasn't sure about: would you rather see this as a single PR for all six, or split into three (one per category)? The categories are small enough that a single PR feels right to me, but splitting makes each piece more independently revertable.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in manim/_config/utils.py at the ignores on lines 432, 1145, 1149, 1156, 1160, and 1293, then run mypy on that file to inspect the reported errors. Use tests/test_config.py for regression coverage; done means the six described ignores are removed, mypy is clean for the file, and the existing tests still pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- tooling
- Issue type
- Refactor
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100