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

Parameter color in ColorClip #366

Closed
furas opened this issue Dec 19, 2016 · 9 comments
Closed

Parameter color in ColorClip #366

furas opened this issue Dec 19, 2016 · 9 comments
Assignees
Labels
documentation Related to documentation in official project docs or individual docstrings.

Comments

@furas
Copy link

furas commented Dec 19, 2016

Documentation shows example with color parameter

clip = ColorClip(size=(460,380), color=[R,G,B])

http://zulko.github.io/moviepy/getting_started/clips.html

but code expects col parameter

def __init__(self, size, col=(0, 0, 0), ismask=False, duration=None):

https://github.com/Zulko/moviepy/blob/master/moviepy/video/VideoClip.py#L1036

@tburrows13
Copy link
Collaborator

Can confirm. Can we change col to color in the code please? Readability counts

@keikoro
Copy link
Collaborator

keikoro commented Feb 17, 2017

@Gloin1313 I think I'd rather col were kept and the documentation adapted because people spell the word differently (colour vs. color). Using the shortcut gets rid of the problem.

@keikoro keikoro added the documentation Related to documentation in official project docs or individual docstrings. label Feb 17, 2017
@tburrows13
Copy link
Collaborator

Well, I would naturally spell it colour, but I am well accustomed to spelling it color for anything like this, where I'd expect American spellings to prevail. Still, I don't really mind either way.

@keikoro
Copy link
Collaborator

keikoro commented Feb 17, 2017

I'd also spell it colour but I think being able to switch effortlessly is something that's easier for some than it is for others (could depend on everyday use, or even general level of English). I have definitely got this wrong in CSS despite knowing how I'm supposed to spell it!

I'm also not sure if the spelling is consistent (edit: consistenly A.E. or B.E.) throughout the entire codebase and documentation (I only know I've seen what looked like Frenchisms to me).

@ghost
Copy link

ghost commented Feb 17, 2017

Why not color and colour as parameters? We would complain if both vars are set and they are not equal.

@tburrows13
Copy link
Collaborator

If no-one objects, I'll write this. I'll also include a DeprecationWarning if col is used.

@tburrows13 tburrows13 self-assigned this Feb 18, 2017
@Zulko
Copy link
Owner

Zulko commented Feb 18, 2017

Can we go for color (and color only) ? col is not "verbose" enough. colour is less used. Having both color and colour would be confusing.

@tburrows13
Copy link
Collaborator

Sure. But I think we need a Deprecation error is col is used, because we are changing the API here.

@ghost
Copy link

ghost commented Mar 7, 2017

closing.. PR 424 was merged into repo to fix this issue.

@ghost ghost closed this as completed Mar 7, 2017
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Related to documentation in official project docs or individual docstrings.
Projects
None yet
Development

No branches or pull requests

4 participants