Skip to content

Provide arguments to mencoder in a more proper way #4004

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

Merged
merged 2 commits into from
Jan 16, 2015

Conversation

pupssman
Copy link
Contributor

As discussed in #4003

Regarding whitespace changes -- PEP8 is violated like everywhere, can't stand it.

@pupssman
Copy link
Contributor Author

Need second look at the test suit to find out what tests do I turn on.

@tacaswell tacaswell added this to the v1.5.x milestone Jan 15, 2015
@dopplershift
Copy link
Contributor

I wouldn't be surprised if I never checked passing anything beyond codec to mencoder. I can confirm the problem here with master and that this pr fixes the problem on my system, so +1 from me.

It would be nice if this could get our smoketests working....

@dopplershift
Copy link
Contributor

I'll add that it doesn't look like our simple smoketest exercises any additional movie generation parameters, so it wouldn't have caught this.

Also, we don't even try to install mencoder on Travis--I'm not sure if that's an oversight or intentional.

@WeatherGod
Copy link
Member

Didn't we try to do that at one point, but gave up due to weird problems?

On Thu, Jan 15, 2015 at 1:05 PM, Ryan May notifications@github.com wrote:

I'll add that it doesn't look like our simple smoketest exercises any
additional movie generation parameters, so it wouldn't have caught this.

Also, we don't even try to install mencoder on Travis--I'm not sure if
that's an oversight or intentional.


Reply to this email directly or view it on GitHub
#4004 (comment)
.

@dopplershift
Copy link
Contributor

All I can find is #2678 , which doesn't indicate that it's ever tried. I can throw up a PR and see what happens.

@tacaswell
Copy link
Member

My only memory of that is memcoder was being difficult and went with the low hanging fruit of testing everything else.

Either we fixed something else sense then (doubtful) or I am/was confused (likely) as the tests seem to work now.

@dopplershift
Copy link
Contributor

Either way, we can discuss anything else about the mencoder tests in #4005, and just leave this about the fixes for commandline args. Does anyone have any objections to this PR?

@tacaswell
Copy link
Member

Probably should be back-ported to 1.4.x

tacaswell added a commit that referenced this pull request Jan 16, 2015
BUG : Provide arguments to mencoder in the proper way
@tacaswell tacaswell merged commit dc90439 into matplotlib:master Jan 16, 2015
tacaswell added a commit that referenced this pull request Jan 16, 2015
BUG : Provide arguments to mencoder in the proper way
@tacaswell
Copy link
Member

cherry-picked as 09be64e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants