Checklist 1: I have no idea how I would create a pytest for this.

Checklist 3,4: I want to add the loop parameter to the PillowWrite.__init__ docstring, but I believe that I would have to copy the entire docstring from MovieWriter.__init__, and then add the extra parameter, so that the sphinx-autodoc will work properly. That's that best way I can think of doing it, is it?

Checklist 5: It's a minor new feature. So, I shouldn't add this?
Checklist 6: It is an api change, but it shouldn't break anything? Like who relied on a gif that only looped once?

1. can you interrogate the gif header for the loop parameter? Probably not so important for a test here.

3/4 Maybe just in the MovieWriter doc string say that loop exists if the writer is Pillow. For triple bonus points you could look into the other writers and see if there is a loop method for those.
5/6 I think a whats new entry is good, but no need for an API entry - thats generally only if API is changed, not if some is added.

Oh, well in that case it’d be good to quickly go through the writers and document their behaviour.

I thought that I'd take a crack a setting the number of loops with ImageMagickWriter. I attempted to modify this line, but the gif still looped infinitely despite this doc. I'll stick with just modifying PillowWriter.

Warning, treated as error:
/home/circleci/project/doc/users/next_whats_new/endless_gif.rst:document isn't included in any toctree

I think this is just since the next_whats_new toctree is commented out in whats_new.rst

I think you could accept a Boolean and convert to an integer internally; just check if a Boolean. OTOH it’s a bit confusing that 0 is loop and 1 is loop only once. Agree that specifying the number loops is desiarable option

I suppose I could do

self.loop = not loop if isintance(loop, bool) else loop

then set the default to loop=True. Then in whatever docstring say that it accepts a Boolean or an integer that specifies number of loops. This way it is clear that True is endless, False is once, and an integer >=1 is an integer.

What do people think?

More that I think about it maybe just make it an integer and value error on anything else so the Boolean doesn’t get passed by accident. Maybe just lay out the options here and folks can vote 🗳 (I’d do it but am on phone). Sorry to drag out a simple change but may as well get it right.

Option 1:
Booleans only.
Pros:

• Simple.

Cons:

• can't set a finite >= 1 integer of loops to display.

Implemented as

self.loop = not loop

Option 2:
ints (this is what it currently is, but without the ValueError)
Pros:

• Can set a finite >= 1 integer of loops to display.

Cons:

• 0 == infinity

Implemented as

# can't use isinstance since bools are ints
if type(loop) == int:
 self.loop = loop
else:
 raise ValueError("loop must be an int")

Option 3:
Both bools and ints
Pros:

• simple True is endless and False plays once
• finite number of loops is possible

Cons:

• We have to explain that both are excepted.

Implemented as

self.loop = not loop if isinstance(loop, bool) else loop

Documentation will be updated to reflect whatever choice.
I'm in favour of option 2 or 3.

I vote 2 + ValueError, just because I think loop=False will actually pass loop=0 if its not caught, but I may be wrong.

I added the ValueError. If we did want to support both bools and ints in the future, then it is easier to add that, then it is to remove support for bools after the fact.

One other question on further thought: since this is not a boolean flag, but a number of loops, should the parameter be loops rather than loop?

That's not a bad point. But I think that the plural loops makes the fact that a value of 0 results in endless looping make even less sense. Pillow uses the parameter name loop, maybe we should just stick with that.

I am looking through the animation module at the moment and based on the available options in all writers at the moment I would say that we could just add unlimited looping also in the pillow writer without any option to change it. Most people want it anyway (but I am not against the option).

I think that a refractoring or at least change in documentation about kwargs should be done in the animation module. At the moment the parameters to the pillow writer are something like,
codec, fps, bitrate, extra_args, metadata but the only one that are not ignored is fps.

So a new API or documentation that clearly show what options that are used in a writer and adding new relevant options should be good.

I have started to change some of the documentation to show which kwargs that are actually used and how.

I would suggest to just add the unlimited looping functionality in a pr and the rest in other prs. Do this really need a test?

I thought that I'd take a crack a setting the number of loops with ImageMagickWriter. I attempted to modify this line, but the gif still looped infinitely despite this doc. I'll stick with just modifying PillowWriter.

Changing that line worked for me. I think that both the pillow- and imagemagick writer API should be the same.

One possibility that are not perfect but is in accordance with the current implementation is to put the new kwarg in moviewriter and just put better documentation in the subclasses which kwargs that are used in that writer.

When both imagemagick and pillow seems to use ints for number of loops with 0 as infinity I think that we should stick to that notation.

Edit:
I would even say that this pr and the extra kwargs in the HTMLWriter breaks the API. It is with those changes not possible to get the same functionality from anim.save(name, writer=writer_obj, ...) and anim.save(name, writer=string_name_of_writer, ...).

So I believe that all new kwargs should be in MovieWriter and animation.save

Sorry it's been a while.

In the interest in getting this bug fix done (and leaving me time for my thousand other projects), I've removed all API changes. This PR now does 2 things:

1. Makes gifs with PillowWriter loop endlessly. Consistent with ImageMagickWriter and ImageMagickFileWriter.
2. (削除) Adds a simple docstring to PillowWriter. (削除ここまで)

I'll leave any refractoring/API changes to someone more familiar with this API than me. This PR is now the minimum needed to fix #11789. The suggested loop keyword argument can be added later.

Ok, I fixed the typo. I believe the doc failures are due to sphinx 1.8.0.

We should have fixed the doc builds now; can you rebase to see if everything's working again?

Last time I tried to rebase a fork, I really messed up the commit history. Any advice?

You should have branched your fork before submitting this PR. Right now your PR is based on your master branch, and that will make your life hard.

After that, you'd best give https://matplotlib.org/devel/gitwash/index.html a thorough look. In particular pay attention to backing up your branch.

With your setup, I would do:

git checkout master
git checkout -b backup-of-master # this will save your bacon if you mess up.
git checkout master # get back to the changes you made in this PR
git fetch upstream # if you haven't set your 'upstream' remote, then you need to do that first. 
git rebase upstream/master # you don't have any conflicts so this should be smooth
git push origin master --force 

If you mess up:

git checkout master
git reset --hard backup-of-master

will get you locally back to where you were.

This is some redundant branch switching:

git checkout -b backup-of-master # this will save your bacon if you mess up.
git checkout master # get back to the changes you made in this PR

git branch backup-of-master will create a branch and not switch to it.

@jklymak Your advice seems to have worked. Thank you. I've rebased.

Can whoever merges this squash-merge it?