Fixed Optional Progress Bar in cuts/detect_scenes

[scherroman]

Progress bar was previously hard-coded to True.

[coveralls]

Coverage remained the same at 54.081% when pulling 453aba7 on scherroman:cuts-luminosities-optional-progress-bar into 6ba12d0 on Zulko:master.

[coveralls]

Coverage remained the same at 54.081% when pulling 719739f on scherroman:cuts-luminosities-optional-progress-bar into 6ba12d0 on Zulko:master.

[coveralls]

Coverage remained the same at 54.081% when pulling 719739f on scherroman:cuts-luminosities-optional-progress-bar into 6ba12d0 on Zulko:master.

[tburrows13]

Thanks, this looks great! If you can confirm that you've tested it and it all runs fine, I'd be happy to merge.

[mbeacom]

lgtm - We should consider better handling for the progress bar over all. ☹️

[scherroman]

I can confirm this works on my end.

It seems like an odd thing to try and write a test for the presence/absence of a progress bar for detect_scenes, given that there's no actual test for detect_scenes yet.

I did try, but had trouble getting the test case to run properly:

def test_PR_587(capsys):
    red = ColorClip((640, 480), color=(255, 0, 0)).set_duration(1)
    green = ColorClip((640, 480), color=(0, 200, 0)).set_duration(1)
    video = concatenate_videoclips([red, green])

    cuts, luminosities = detect_scenes(video, fps=10, progress_bar=True)
    out, err = capsys.readouterr()
    assert len(err) != 0

    cuts, luminosities = detect_scenes(video, fps=10, progress_bar=False)
    out, err = capsys.readouterr()
    assert len(err) == 0

Normally, pytest should be able to capture any output to stdout and stderr during the execution of a test, so we'd be able to tell if the progress bar was actually being shown or not. However, pytest only seemed to capture the output once the test had completed:

[scherroman]

In any case, I think this would be a good general test case to add for detect_scenes:

from moviepy.editor import ColorClip, concatenate_videoclips
from moviepy.video.tools.cuts import detect_scenes

def test_detect_scenes():
    red = ColorClip((640, 480), color=(255,0,0)).set_duration(1)
    green = ColorClip((640, 480), color=(0,200,0)).set_duration(1)
    video = concatenate_videoclips([red, green])

    cuts, luminosities = detect_scenes(video, fps=10, progress_bar=False)

    assert len(cuts) == 2

I could open up a new pull request with this test, but I'm a little unsure where the test case should live. Perhaps test_compositing.py, or perhaps a new file test_video_tools.py?

Feels like the test suite could benefit from a folder structure mirroring the library. That way one could add a test for detect_scenes under tests/video/tools/test_cuts.py.

[mbeacom]

👍 for tests/video/tools/test_cuts.py

[kencochrane]

Lgtm