Updating animation file writer to allow keywork arguments when using with construct
#6304
Conversation
| @@ -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): |
There was a problem hiding this comment.
can you remove both args and kwargs from this signature? We do not use either of them so having them will just mask errors.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
|
Other than @tacaswell 's comment, it's a 👍 from me. |
|
Thanks! @jmc734 I think this is your first contribution to mpl. Congratulations and hopefully we will see you again 🎉 . |
|
@tacaswell, may I backport this? I view it as fixing a rather frustrating bug, which I tripped over and reported in #7191. |
This is the functionality added to master in 6bac790 (matplotlib#6304). Directly cherry-picking that is not possible because of the intervening introduction of abstract base classes.
Previously, in order to pass configurations to
saving()and subsequentlysetup(), the caller had to provide positional arguments. There is nothing explicitly wrong with that approach though thesaving()method is currently advertised as taking the same parameters assetup(). 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:Instead, you would have to know the order of the arguments and make the call like so:
This is a small change but I think it makes code easier to read and more tolerant to future API change.