`Colorclip` changed `col`>`color` #424

tburrows13 · 2017-02-18T17:31:37Z

Changed the col paramater of ColorClip to color and added a DeprecationWarning if the users tries to use col.

…recationWarning if the users tries to use `col`

keikoro · 2017-02-19T02:19:36Z

Does this fix #366? Dropping the no. in here so this PR is linked from the issue.

tburrows13 · 2017-02-19T13:08:09Z

Sorry, yes, it does.

ghost · 2017-02-22T16:43:36Z

Gloin1313. this looks good to me. Should we go ahead and merge? Anyone object?

Zulko · 2017-02-22T16:49:28Z

Just two remarks: please enforce PEP8, no line with more than 80 characters, it's really important.
Also -less important- maybe in a first time we could raise a warning instead of an error, i don't really like to have a parameter there with no other purpose than throwing an error.

ghost · 2017-02-22T16:51:58Z

@Zulko.. is there a way we can add PEP 8 testing of the source code? I've heard of this, but I haven't tried it myself. Maybe one else has some experience with PEP 8 testing and github.

Zulko · 2017-02-22T16:56:23Z

There are but I've never been really confortable with these, in some cases you have to disregard PEP8 for one line or two, and that makes your tests fail. On moviepy you'll certainly have a lot of red flags. I'd suggest we just keep the basics of PEP8 in mind (line length, naming, etc.) and just eyeball one another's code.

ghost · 2017-02-22T16:59:56Z

good points.. We don't want to get too anal about this.. ;)

keikoro · 2017-02-22T18:04:53Z

@Earney @Gloin1313 Any decent code editor will allow you to use a word wrap column/ruler, which can be set to 78-80 characters (78 being preferred because of the extra spaces just in case).

It's not just useful if you want to follow PEP8, but also for sites like GitHub, where longer lines have to be scrolled horizontally, which is a pain. For the rest, a read-through of the PEP8 page as a refresher should suffice (+ maybe checking back with it a few times for those completely new to it).

tburrows13 · 2017-02-26T19:53:50Z

@Zulko I'll add those suggestions. Sorry about the line length thing, Pycharm seems to think that PEP 8 wants no more than 120 chars, which my line just fits under. I've changed the default settings now to 80 chars.
When you say "warning", do you mean just print("Moviepy WARNING: message here) or do you mean something like the standard library warnings module?
Whichever we do, I'll then add color = col, unless both are specified, in which case I'll use color and have another warning that col is being ignored.

ghost · 2017-02-27T14:59:06Z

@Gloin1313 , we are using the warnings module in /moviepy/moviepy/video/io/ffmpeg_reader.py. So that is probably what @Zulko is referring to.
Can you also create a test or two for this PR and place it in test/test_PR.py ? Let me know if you need some help.

* Fixed copy-paste typo Changed documentation for the color parameter to distinguish from bg_color * Fixed missing list (using python 3) * fixed module hierarchy for Trajectory * fixed addy * small recipe * on_color function docstring has wrong parameter The docstring for the on_color method has a parameter of bg_color but the function uses color as the parameter. * Update README.rst Changed Code Block to use Python Syntax Highlighting * fix deprecation message currently, the docstring is nonsense. ``` The function ``concatenate_videoclips`` is deprecated and is kept temporarily for backwards compatibility. Please use the new name, ``concatenate_videoclips``, instead. ``` * ImageSequenceClip: Check for fps and durations rather than fps and duration * Add a test case. * Add another test * ensures int arguments to np.reshape; closes Zulko#383 * fix issue Zulko#401 * fix issue Zulko#335 * Update maintainer section in README * make concatenate_videoclips Python 3 compatible.. fix issue Zulko#313 * Increment release version * CompositeVideoClip doesn't accept an argument of transparent * move PY3 variable to compat.py * fix movie => moviepy typo * fix issue Zulko#341 * Fixed typo Zulko#375 Zulko#375 fixed * fix issue Zulko#357, which makes real problem more obvious (media file does not exist * Revert "small recipe (mirroring a video)" * Fixed indentation * .gitignore ignore Mac-specific files, Jetbrains settings dir * README.rst make mention of Gitter, add PyPI and Gitter badge * README.rst wording, formatting * README.rst structure link targets, add co-maintainers (with @username) * README.rst move maintainers, contributing sections; change docs x-reference name * README.rst fix grammar * fix issue 145. raise Exception when concatenate method != chain or compose * make PEP8 compatible * fix PR Zulko#413 . (issue Zulko#357) * create test for issue Zulko#145 * add tests/media to .gitignore * fix Issue Zulko#385 , no DirectoryClip class (Zulko#434) * fix issue Zulko#385 , no DirectoryClip class * replace DirectoryClip with ImageSequenceClip * fix issue 417, unicode has no attribute 'shape' error. * add test for issue 417 * add test for issue 417 * Fixed resize documentation issue Zulko#319 (Zulko#346) * Handle bytes when listing fonts in VideoClip.py (Zulko#306) Handle bytes when listing fonts in VideoClip.py * add test for PR306 * add test for PR306 (Zulko#440) * create test file for pull requests (Zulko#433) * Test issue 407 (video has a valid fps after concatenate function) (Zulko#443) * Move PR test to test_PR.py file (Zulko#444) * move PR test from test_issues.py to test_PR.py * add code to download python_logo.png * remove duplicate test_issue_417 function * add testing with travis-ci (Zulko#447) added testing via travis-ci * put DEVNULL into compat.py (Zulko#432) * add travis-ci badge to readme file * pick highest fps when concatenating (Zulko#416) * readers.py cast chunksize from float to int * choose highest fps of clips when concatenating * pick highest fps when concatenating * pick highest fps when concatenating * fps either max or none * remove resolve markers removed resolve markers such as HEAD, etc so that the file will compile correctly. Remove some double blank lines, etc * update concatenate.py; add c.fps is not None * add test for issue 416 * fix test_issue_416

Update

tburrows13 · 2017-02-28T21:59:17Z

This is ready to go. Is it OK if I place the test in a separate PR? The colorclip-update branch doesn't have the test_PR.py file as it was added since I created the branch. EDIT: tests are in this PR

ghost · 2017-02-28T22:53:17Z

Sure.. we can merge the two files together later.

…

On Feb 28, 2017 3:59 PM, "Gloin1313" ***@***.***> wrote: This is ready to go. Is it OK if I place the test in a separate PR? The colorclip-update branch doesn't have the test_PR.py file as it was added since I created the branch. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#424 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABGu8gc-0BswbPROtHTOxxSaMDTKPpqyks5rhJi2gaJpZM4MFMEL> .

tburrows13 · 2017-03-01T10:55:27Z

@Earney About the tests: are they just testing for proper usage, or shall I put one in using col to check the warnings work ok as well?

… into colorclip-update

tburrows13 · 2017-03-01T11:08:24Z

Tests are in this one after all. Are they what you wanted?

coveralls · 2017-03-01T11:10:58Z

Coverage increased (+0.2%) to 32.121% when pulling f50af0a on Gloin1313:colorclip-update into 16d33e3 on Zulko:master.

coveralls · 2017-03-01T17:35:35Z

Coverage increased (+3.9%) to 35.794% when pulling 0395b93 on Gloin1313:colorclip-update into 16d33e3 on Zulko:master.

coveralls · 2017-03-01T17:38:48Z

Coverage increased (+3.9%) to 35.794% when pulling 0395b93 on Gloin1313:colorclip-update into 16d33e3 on Zulko:master.

ghost · 2017-03-01T18:29:48Z

Nice work.. unless someone objects, I'd merge this into the repo. Maybe give others a few days to respond (maybe till friday?)

tburrows13 · 2017-03-01T19:52:49Z

There are a lot of commits in the PR. Is the right thing to do "rebase and merge" instead of just "Merge pull request"? Or do we "squash and merge"?

ghost · 2017-03-01T20:36:22Z

A squash and merge will do what u want.

tburrows13 and others added 2 commits February 18, 2017 17:24

Changed the col paramater of ColorClip to color and added a Dep…

87699df

…recationWarning if the users tries to use `col`

Update VideoFileClip.py

85d1cc6

tburrows13 and others added 6 commits February 27, 2017 16:35

Added warnings

e9d1b00

Merge pull request #5 from Zulko/master

cd06bc1

Update

Merge branch 'master' into colorclip-update

0dd5623

Update VideoFileClip.py

41691e9

Merge branch 'master' of https://github.com/Zulko/moviepy

587c6a0

tburrows13 added 3 commits March 1, 2017 10:59

Merge branch 'master' into colorclip-update

22646a3

Merge branch 'colorclip-update' of https://github.com/Gloin1313/moviepy…

b62b117

… into colorclip-update

Added test

f50af0a

tburrows13 added 2 commits March 1, 2017 17:30

Added return for PEP 8 compliance

17b0441

Change

0395b93

tburrows13 merged commit 01f207a into Zulko:master Mar 2, 2017

tburrows13 deleted the colorclip-update branch March 2, 2017 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Colorclip` changed `col`>`color` #424

`Colorclip` changed `col`>`color` #424

tburrows13 commented Feb 18, 2017

keikoro commented Feb 19, 2017

tburrows13 commented Feb 19, 2017

ghost commented Feb 22, 2017

Zulko commented Feb 22, 2017

ghost commented Feb 22, 2017

Zulko commented Feb 22, 2017

ghost commented Feb 22, 2017

keikoro commented Feb 22, 2017 •

edited

Loading

tburrows13 commented Feb 26, 2017 •

edited

Loading

ghost commented Feb 27, 2017

tburrows13 commented Feb 28, 2017 •

edited

Loading

ghost commented Feb 28, 2017 via email

tburrows13 commented Mar 1, 2017

tburrows13 commented Mar 1, 2017 •

edited

Loading

coveralls commented Mar 1, 2017 •

edited

Loading

coveralls commented Mar 1, 2017 •

edited

Loading

coveralls commented Mar 1, 2017 •

edited

Loading

ghost commented Mar 1, 2017

tburrows13 commented Mar 1, 2017

ghost commented Mar 1, 2017

Colorclip changed col>color #424

Colorclip changed col>color #424

Conversation

tburrows13 commented Feb 18, 2017

keikoro commented Feb 19, 2017

tburrows13 commented Feb 19, 2017

ghost commented Feb 22, 2017

Zulko commented Feb 22, 2017

ghost commented Feb 22, 2017

Zulko commented Feb 22, 2017

ghost commented Feb 22, 2017

keikoro commented Feb 22, 2017 • edited Loading

tburrows13 commented Feb 26, 2017 • edited Loading

ghost commented Feb 27, 2017

tburrows13 commented Feb 28, 2017 • edited Loading

ghost commented Feb 28, 2017 via email

tburrows13 commented Mar 1, 2017

tburrows13 commented Mar 1, 2017 • edited Loading

coveralls commented Mar 1, 2017 • edited Loading

coveralls commented Mar 1, 2017 • edited Loading

coveralls commented Mar 1, 2017 • edited Loading

ghost commented Mar 1, 2017

tburrows13 commented Mar 1, 2017

ghost commented Mar 1, 2017

`Colorclip` changed `col`>`color` #424

`Colorclip` changed `col`>`color` #424

keikoro commented Feb 22, 2017 •

edited

Loading

tburrows13 commented Feb 26, 2017 •

edited

Loading

tburrows13 commented Feb 28, 2017 •

edited

Loading

tburrows13 commented Mar 1, 2017 •

edited

Loading

coveralls commented Mar 1, 2017 •

edited

Loading

coveralls commented Mar 1, 2017 •

edited

Loading

coveralls commented Mar 1, 2017 •

edited

Loading