Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fluent API for effects (monkeypatch as methods) #1105

Merged
merged 26 commits into from
Oct 8, 2020

Conversation

mgaitan
Copy link
Collaborator

@mgaitan mgaitan commented Mar 24, 2020

This PR is a (little bit) cleaner implementation to patch "functions effects" as methods. Supersedes #1104

Basically, avoid to use exec and deprecates fx.all modules, importing every function in each package's init.

In addition, it defines __all__ to reduce the namespace pollution when import from moviepy.editor import *

@tburrows13 tburrows13 added the refactor Does not affect the end user at all i.e. making code easier to read or PEP8 compliant. label Mar 24, 2020
@coveralls
Copy link

coveralls commented Mar 25, 2020

Coverage Status

Coverage increased (+0.2%) to 68.044% when pulling eee1e1f on mgaitan:fluent_api into 4356fcd on Zulko:master.

@tburrows13 tburrows13 added this to the Release v2.0.0 milestone Mar 26, 2020
@tburrows13 tburrows13 closed this Mar 29, 2020
@mgaitan
Copy link
Collaborator Author

mgaitan commented Mar 29, 2020

what happened here? you closed this in favor of something else?

@tburrows13
Copy link
Collaborator

Not intentional, sorry!

@tburrows13
Copy link
Collaborator

Oh, it closed itself when I deleted the v2 branch. Apparently there's no way to change the base now (???). You might have to make a new one, sorry.

@mgaitan
Copy link
Collaborator Author

mgaitan commented Mar 29, 2020

ack, no problem.

@tburrows13
Copy link
Collaborator

I think it's all sorted now, you can ignore this little fiasco :)

@tburrows13 tburrows13 reopened this Mar 30, 2020
@tburrows13 tburrows13 changed the base branch from v2 to master March 30, 2020 11:35
@mgaitan
Copy link
Collaborator Author

mgaitan commented Apr 4, 2020

@tburrows13 do you have an opinion about this?

It's much humble that what I had in mind, that basically was do this patching via a metaclass to avoid the need to import the "patched classes" from a different place (i.e avoid editor.py at all). I explored that idea a bit, but I then I realized it's not as easy I thought because of circular dependencies (after all, we are patching the "base" classes),

However I guess it's cleaner enough for now, and much more pythonic than the execs and all.py modules.

The unique downside (very minor IMHO) is that fx namespaces changed if for some reason you import effects directly from the packages.

In master if you import something from the fx package, you get the module

In [1]: from moviepy.video.fx import mirror_x                                                                                                                                                 

In [2]: mirror_x?                                                                                                                                                                             
Type:        module
String form: <module 'moviepy.video.fx.mirror_x' from '/home/tin/lab/moviepy/moviepy/video/fx/mirror_x.py'>
File:        ~/lab/moviepy/moviepy/video/fx/mirror_x.py
Docstring:   <no docstring>

and here you get the actual fx function for the same import

In [3]: from moviepy.video.fx import mirror_x                                                                                                                                                 

In [4]: mirror_x?                                                                                                                                                                             
Signature: mirror_x(clip, apply_to='mask')
Docstring: flips the clip horizontally (and its mask too, by default) 
File:      ~/lab/moviepy/moviepy/video/fx/mirror_x.py
Type:      function

However, this way still works in the new implementation

In [1]: from moviepy.video.fx.mirror_x import mirror_x                                                                                                                                        

In [2]: mirror_x?                                                                                                                                                                             
Signature: mirror_x(clip, apply_to='mask')
Docstring: flips the clip horizontally (and its mask too, by default) 
File:      ~/lab/moviepy/moviepy/video/fx/mirror_x.py
Type:      function

so, we are mostly ok.

moviepy/editor.py Outdated Show resolved Hide resolved
@tburrows13
Copy link
Collaborator

tburrows13 commented Apr 6, 2020

This is definitely better than what we have currently, and better than #1104. Thanks for detailing how this change affects importing from the fx package, but I don't think that that should be a major concern, just something to point out in the changelog (which I can write before merging).

I wouldn't be opposed to getting rid of editor.py altogether as it is rather unpythonic, but as you point out, it might be hard to still keep the fx as methods of the classes. I'm not that familiar with package-level things, but one idea that I had was to move all of editor.py into __init__.py, so users just import moviepy or from moviepy import *. Having just tested it, it allows doing from moviepy.video.io.VideoFileClip import VideoFileClip, and then still being able to use VideoFileClip.rotate() (which is only added as a method in moviepy/__init__.py()). I think that we could go even further and prohibit importing directly from moviepy (by editing __all__?) to enforce standard pythonic (is it?) importing directly from their relevant submodules, but still do the fx as methods stuff in moviepy/__init__.py().

Perhaps that last paragraph should be a different issue/PR, and that last sentence would be quite a bit more of a drastic change and might require reworking the hierarchy of moviepy if everyone is accessing the classes from their proper locations. Furthermore, I still don't know if doing things in moviepy/__init__.py() is any more pythonic than this PR.

Your thoughts would be appreciated :)

@tburrows13 tburrows13 added feature New addition to the API i.e. a new class, method or parameter. and removed refactor Does not affect the end user at all i.e. making code easier to read or PEP8 compliant. labels Apr 8, 2020
@tburrows13
Copy link
Collaborator

Closes #591

@tburrows13
Copy link
Collaborator

Ok, here's my current thinking.

Move most of the standard imports and the fx 'trickery' into the main __init__.py, and recommend that standard moviepy programs just import what they need from moviepy, not moviepy.editor.
However, leave the stuff in moviepy.editor that only makes sense when doing 'live' editing, such as sliders (which depends on matplotlib), preview and show (which depend on Pygame) and the ipython stuff. This means that import moviepy.editor should only be used for its original intended purpose, which is live editing of movies, and importing moviepy (without editor) should take less than half the time and it won't initialise a Pygame process.

We can also look at including extra imports into either __init__.py or editor.py that are not currently in editor.py where appropriate.

@Zulko @mgaitan I'd appreciate your feedback on this.

@tburrows13 tburrows13 merged commit 4423b16 into Zulko:master Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New addition to the API i.e. a new class, method or parameter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants