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

Backwards compatibility #889

Closed
das7pad opened this issue Dec 17, 2018 · 6 comments
Closed

Backwards compatibility #889

das7pad opened this issue Dec 17, 2018 · 6 comments
Labels
project-admin Anything to do with the administration & organisation of moviepy. I.e. project "meta".

Comments

@das7pad
Copy link
Contributor

das7pad commented Dec 17, 2018

Commit bfad5ea introduces a non backwards compatible change for the progress-bar/logger.

See this line as an example: bfad5ea#diff-4b7e3f0e11cb6f470780d69172d8fbe1R144

Related downstream Issue:
das7pad/hangoutsbot#74
Travis build:
https://travis-ci.org/das7pad/hangoutsbot/jobs/469040908#L806

I am working on a fix. PR coming later today.

Sneak

diff --git a/moviepy/audio/io/ffmpeg_audiowriter.py b/moviepy/audio/io/ffmpeg_audiowriter.py
index 5f70425..29c84cf 100644
--- a/moviepy/audio/io/ffmpeg_audiowriter.py
+++ b/moviepy/audio/io/ffmpeg_audiowriter.py
@@ -1,11 +1,11 @@
 import subprocess as sp
 import os
-import proglog
 
 from moviepy.compat import DEVNULL
 
 from moviepy.config import get_setting
 from moviepy.decorators import requires_duration
+from moviepy.logger import get_logger, PROGRESS_BAR_SENTINEL
 
 
 class FFMPEG_AudioWriter:
@@ -144,19 +144,20 @@ class FFMPEG_AudioWriter:
 def ffmpeg_audiowrite(clip, filename, fps, nbytes, buffersize,
                       codec='libvorbis', bitrate=None,
                       write_logfile=False, verbose=True,
-                      ffmpeg_params=None, logger='bar'):
+                      ffmpeg_params=None, progress_bar=PROGRESS_BAR_SENTINEL,
+                      logger='bar'):
     """
     A function that wraps the FFMPEG_AudioWriter to write an AudioClip
     to a file.
 
     NOTE: verbose is deprecated.
     """
+    logger = get_logger(progress_bar, logger)
 
     if write_logfile:
         logfile = open(filename + ".log", 'w+')
     else:
         logfile = None
-    logger = proglog.default_bar_logger(logger)
     logger(message="MoviePy - Writing audio in %s")
     writer = FFMPEG_AudioWriter(filename, fps, nbytes, clip.nchannels,
                                 codec=codec, bitrate=bitrate,
diff --git a/moviepy/logger.py b/moviepy/logger.py
index e69de29..5cb57b3 100644
--- a/moviepy/logger.py
+++ b/moviepy/logger.py
@@ -0,0 +1,10 @@
+import proglog
+
+PROGRESS_BAR_SENTINEL = object()
+
+
+def get_logger(progress_bar, logger):
+    if progress_bar is not PROGRESS_BAR_SENTINEL:
+        logger = 'bar' if progress_bar else None
+
+    return proglog.default_bar_logger(logger)

@Zulko
Copy link
Owner

Zulko commented Dec 17, 2018

Yes this is non-backwards compatible, I documented it in the README. I am not interested in maintaining backwards compatibility for the progress_bars and verbose parameters, but I did mess up the versionning by initially only bumping the minor number (0.2.3.5 is the last compatible version. 0.2.3.6 is not), so I am open to suggestions if that is a problem to you.

If you are using moviepy for a web service I suggest you adopt the new API, the new system allows to send progress feedback to a webpage or any UI, which the former system didnt allow.

@das7pad
Copy link
Contributor Author

das7pad commented Dec 17, 2018

I would expect a bump of the major version for a backwards incompatible change.

Sure it is your project, but keep in mind that 1100 other projects dependent on it on github.
Each of them might need to change their access on your public API now. Or might have to add a middleware to change the api-access based on the installed version of moviepy.

@Zulko
Copy link
Owner

Zulko commented Dec 17, 2018

Ok, I just removed yesterday's releases from PyPI (I had no idea this was possible \o/), it is back to 0.2.3.5 now.

I'll release the new changes under 0.3.0.0, does that make sense ?

Still, I would advise that you do like most MoviePy users and freeze the moviepy version in the requirements. It certainly got better in the last few years because the new maintainers have a better clue of versionning (and I do too, but this time I was just tired), but there are still only 65% code coverage at the moment so you are never protected against regressions.

@tburrows13
Copy link
Collaborator

tburrows13 commented Dec 17, 2018

I’d agree with releasing the changes as 0.3, or even push the version number to 1.0 and adopt semver (semver.org)? Well done for implementing the progress bar update! There are a lot of threads about it I’m various places that can now be cleaned up: https://github.com/Zulko/moviepy/projects/3#column-2198990

@das7pad
Copy link
Contributor Author

das7pad commented Dec 20, 2018

Adopting to semantic versioning sounds great.

@keikoro keikoro added the project-admin Anything to do with the administration & organisation of moviepy. I.e. project "meta". label Dec 30, 2018
@das7pad
Copy link
Contributor Author

das7pad commented Mar 13, 2019

Fixed in b0f0ec6 (version bump 1.0.0).

@das7pad das7pad closed this as completed Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project-admin Anything to do with the administration & organisation of moviepy. I.e. project "meta".
Projects
None yet
Development

No branches or pull requests

4 participants