Updating animation file writer to allow keywork arguments when using `with` construct #6304

Merged
merged 3 commits into from Apr 29, 2016

Conversation

Projects
None yet
7 participants
Contributor

jmc734 commented Apr 14, 2016

Previously, in order to pass configurations to saving() and subsequently setup(), the caller had to provide positional arguments. There is nothing explicitly wrong with that approach though the saving() method is currently advertised as taking the same parameters as setup(). For example, if you wanted to disable temporary file clearing for a FileMovieWriter, you previously would not have been able to structure it like this:

with recorder.saving(fig, capFile, capDPI, tempPrefix, clear_temp = False):

Instead, you would have to know the order of the arguments and make the call like so:

with recorder.saving(fig, capFile, capDPI, tempPrefix, False):

This is a small change but I think it makes code easier to read and more tolerant to future API change.

@jmc734 jmc734 Updated to allow keyword args to be passed when using the `with savin…
…g()` construct
299f8af

mdboom added the needs_review label Apr 14, 2016

@tacaswell tacaswell and 2 others commented on an outdated diff Apr 14, 2016

lib/matplotlib/animation.py
@@ -246,7 +246,7 @@ def frame_size(self):
width_inches, height_inches = self.fig.get_size_inches()
return width_inches * self.dpi, height_inches * self.dpi
- def setup(self, fig, outfile, dpi, *args):
+ def setup(self, fig, outfile, dpi, *args, **kwargs):
@tacaswell

tacaswell Apr 14, 2016

Owner

can you remove both args and kwargs from this signature? We do not use either of them so having them will just mask errors.

@WeatherGod

WeatherGod Apr 15, 2016

Member

Wouldn't removing *args be a potential compatibility break? Admittedly, I am having a hard time imagining a scenario where one would have had extra arguments down to this point, but I just wanted to make sure we aren't doing something blindly. It would be nice if I could remember why we had *args there in the first place...

@jmc734

jmc734 Apr 18, 2016

Contributor

It was added way back in 2012 in the original work-in-progress commit for animation file writers. Unfortunately, that commit has no further documentation other than the code changes themselves. I would venture a guess though that the reasoning was that saving() took arbitrary arguments and called setup() with them so, if for some reason an overriden setup() called the super setup() with the entire set of arbitrary arguments, it wouldn't break. If that is true then I think we should remove it as that case is unlikely and would be kind of dirty anyway.

Contributor

dopplershift commented Apr 14, 2016

Other than @tacaswell 's comment, it's a 👍 from me.

jmc734 added some commits Apr 18, 2016

@jmc734 jmc734 Merge remote-tracking branch 'upstream/master'
4d8bcb8
@jmc734 jmc734 Removed arbitrary argument lists from setup()
653e240

tacaswell added this to the 2.1 (next point release) milestone Apr 29, 2016

@tacaswell tacaswell merged commit 6bac790 into matplotlib:master Apr 29, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

tacaswell removed the needs_review label Apr 29, 2016

Owner

tacaswell commented Apr 29, 2016

Thanks!

@jmc734 I think this is your first contribution to mpl. Congratulations and hopefully we will see you again 🎉 .

Owner

efiring commented Sep 28, 2016

@tacaswell, may I backport this? I view it as fixing a rather frustrating bug, which I tripped over and reported in #7191.

@efiring efiring added a commit to efiring/matplotlib that referenced this pull request Sep 30, 2016

@efiring efiring BUG: 'saving' context manager now passes all args, kw to setup.
This is the functionality added to master in 6bac790 (#6304).
Directly cherry-picking that is not possible because of the intervening
introduction of abstract base classes.
137fd6d

QuLogic added the Animation label Sep 30, 2016

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