Add missing mobject method chaining - #4913
Conversation
|
The tests don't fail for me locally: (after installing the |
|
Cool, thank you! Looks like you are running into an issue with some of our doctests; from the ubuntu / Python 3.13 CI: The windows CIs fail with a weird path issue, I can take a closer look at that later. 🧐 |
|
Also, I noticed that def add_points_as_corners(self, points):
...
return pointsI didn't change that as it would change the return type, but returning |
|
A thought: Some of these methods are nasty when used in an This is nothing against this PR - I note that |
behackl
left a comment
There was a problem hiding this comment.
Thanks for your PR! I've pushed one commit to fix some issues with CI, the docbuild is apparently failing now -- don't think it is generally flaky right now so might be due to changes made on this PR. Some comments:
- I think it is fine to technically support chaining, even if some methods are not suitable for the .animate syntax. This is a somewhat orthogonal problem that we can think about; perhaps we can decorate non-suitable methods to let the AnimationBuilder fail or so... but it doesn't matter for now.
VMobject.align_pointshas an "overly eager" early return which currently doesn't returnself(you fixed it forOpenGLVMobject).- Plus I found two inline functions, see comments below, where the change should be reverted.
I'd be in favour of changing the output of OpenGLVMobject.add_points_as_corners as well, only makes sense to clean this up. I'll need to do a somewhat more rigorous review round later, but for now this is it. Thanks again for your efforts!
Agree! There already exists some precedent for disallowing |
Some of the things you reverted were suggested to me by |
Oh, that is very weird: in particular the change with the symlink should definitely not happen. We should probably cleanup our pre-commit setup a bit. |
…chaining # Conflicts: # manim/mobject/graphing/probability.py # manim/mobject/table.py
behackl
left a comment
There was a problem hiding this comment.
Resolved the conflicts + pushed an additional commit fixing three more missing returns. Looks all good for me now, happy to get this merged once the pipeline passes.
Thanks again for your efforts!
Overview: What does this pull request change?
Adds missing method chaining for mobject methods.
-> Selftype annotation.-> [MobjectClass]to-> Self.return selffor updater-methods that returned nothing.Reviewer Checklist