Skip to content

Raise NotImplementedError in MovieWriter._args instead of returning it#32067

Open
jameslin9968 wants to merge 1 commit into
matplotlib:mainfrom
jameslin9968:fix-movie-writer-args-raise
Open

Raise NotImplementedError in MovieWriter._args instead of returning it#32067
jameslin9968 wants to merge 1 commit into
matplotlib:mainfrom
jameslin9968:fix-movie-writer-args-raise

Conversation

@jameslin9968

@jameslin9968 jameslin9968 commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR summary

MovieWriter._args() is a placeholder meant to be overridden by concrete
subclasses. It used return NotImplementedError(...) instead of raise, so a
MovieWriter subclass that forgot to override it would not fail loudly:
_run() does command = self._args(), which would silently bind an exception
instance and pass it downstream as if it were the command-line argument list,
producing a confusing error instead of the intended "args needs to be
implemented by subclass" message. This changes return to raise and adds a
regression test.

Minimal reproduction of the previous behavior:

from matplotlib.animation import MovieWriter
# _args() returned the exception object instead of raising it:
result = MovieWriter._args(MovieWriter.__new__(MovieWriter))
assert isinstance(result, NotImplementedError)  # bug: returned, not raised

Note (per review discussion): making _args an @abc.abstractmethod was
considered but rejected — HTMLWriter is a non-subprocess MovieWriter that
never calls _run/_args and has no _args, so an abstractmethod would make
it un-instantiable (18 test failures). A proper fix is the _run/_args mixin
refactor already flagged by the source TODO, which is out of scope for this
one-line bugfix.

AI Disclosure

This PR was authored with substantial AI assistance (Claude). AI was used to
localize the issue in MovieWriter._args, write the one-line fix, and generate
the regression test in test_animation.py. All changes were run and verified
locally: the new test fails without the fix and passes with it, and the full
test_animation.py suite passes (48 passed / 23 skipped / 0 failed).

PR checklist

  • [N/A] "closes #0000" — no pre-existing issue was filed for this
  • new and changed code is tested (regression test in test_animation.py)
  • [N/A] Plotting related features demonstrated in an example — bugfix, no new feature
  • [N/A] New Features / API Changes release note — behavior-only bugfix, no API change
  • Documentation complies with guidelines (no doc changes needed)

@jameslin9968
Raise NotImplementedError in MovieWriter._args instead of returning it
6edcbc5 
MovieWriter._args() is a placeholder meant to be overridden by concrete
subclasses. It used `return NotImplementedError(...)` instead of `raise`,
so a MovieWriter subclass that forgot to override it would not fail loudly:
_run() calls `command = self._args()`, which would silently bind an
exception instance and pass it on as if it were the command-line argument
list, producing a confusing downstream error instead of the intended
'args needs to be implemented by subclass' message.

Add a regression test.
@timhoffm

timhoffm commented Jul 18, 2026

Copy link
Copy Markdown
Member

Is there a reason to not make this an @abc.abstractmethod?

@jameslin9968

jameslin9968 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @timhoffm — I tried @abc.abstractmethod and it turns out not to work cleanly here.

HTMLWriter subclasses MovieWriter (via FileMovieWriter) but is not subprocess-based: it overrides setup/grab_frame/finish to write frames directly as HTML/base64 and never calls _run/_args. Marking _args as @abc.abstractmethod makes HTMLWriter non-instantiable, which breaks it (and the animation test suite — 18 failures locally).

This is exactly the case flagged by the existing TODO in MovieWriter ("still an abstract class and needs to be extended with a mixin"): the subprocess machinery (_run/_args) does not belong on the common base shared by non-subprocess writers like HTMLWriter. Doing it properly would mean extracting a _SubprocessWriterMixin that only subprocess writers inherit — a larger refactor beyond the scope of this one-line fix.

So for this PR I have kept the conservative return → raise change. Happy to open a separate PR for the mixin refactor if you think it is worthwhile.

@jameslin9968
jameslin9968 force-pushed the fix-movie-writer-args-raise branch from 3936274 to 6edcbc5 Compare July 19, 2026 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timhoffm timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

topic: animation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jameslin9968 @timhoffm