>>> from itertools import repeat
>>> repeat(2, -10).__length_hint__()
0
>>> repeat(2, times=-10).__length_hint__()
18446744073709551606
>>> repeat(2, times=-10)
repeat(2, -10)
>>> repeat(2, -10)
repeat(2, 0)

Hereby, I attached two alternatives of patch to make the behaviour consistent.

The first one makes the negative number parameter/keyword ALWAYS means endless.

The second patch makes the negative number parameter/keyword ALWAYS means 0.

The preferred behavior is that a negative number always means 0.

That is what lists do:

    >>> [1] * (-5)
    []

Improved the patch which makes negative number *always* means 0. Added comment and put more test.

Raymond's patch looks even more cumbersome than previous code. And it doesn't work in some cases:

>>> it = itertools.cycle([-2, -1])
>>> class Index:
...     def __index__(self): return next(it)
... 
>>> itertools.repeat(42, times=Index())
repeat(42, -1)

Current behaviors 3.4b2
>>> itertools.repeat('a', -1)
repeat('a', 0)
>>> itertools.repeat('a', times=-1)
repeat('a')  # forever
>>> itertools.repeat('a', times=-2)
repeat('a', -2)

My opinions (same as what Raymond says -- negative == 0)
The first line is correct in both behavior and representation.
The second line behavior (and corresponding repr) are wrong.
The third line repr is wrong but the behavior is like the first.

Guido's response on pydev, in context about having a signature that can be captured by C and Python. The main different is comment about adding None to the C signature in addition to the Python 'equivalent, which has 'times=None'.

"If I had complete freedom in redefining the spec I would treat
positional and keyword the same, interpret absent or None to mean
"forever" and explicit negative integers to mean the same as zero, and
make repr show a positional integer >= 0 if the repeat isn't None.

But I don't know if that's too much of a change."

I think the only thing different is comment about None.

This problem been independently rediscovered by people converting
code to Argument Clinic.  A Python signature can't express these
semantics, where a parameter behaves differently depending on
whether it's passed in by keyword or by reference.  So Argument
Clinic can't either.  I think it would be best if itertools.repeat
behaved like a pure Python function--that is, that it behaved
the same whether "times" was passed in by position or by keyword.

What's I find curious: the documentation is wildly out of sync with the implementation.  It says:

    itertools.repeat(object[, times])
    ....
    def repeat(object, times=None):
        ....

    http://docs.python.org/3.4/library/itertools.html#itertools.repeat

But repeat() doesn't support passing in None for the times parameter,
if indeed it ever has.


I see two possible choices here.

1) Honor the existing behavior.  Change the signature to simply

   def repeat(object, times=-1):

   and document it that way.

2) Honor the documentation.  Change the implementation to

   def repeat(object, times=None):

   This change could break code.  So we'd have to go through a
   deprecation cycle.  Breaking "times=-1" without a deprecation
   cycle is simply not viable at this point.

I could live with either.

Serhiy said, "Why do you cast PyDict_Size(kwds) to int?"

Sorry, Serhiy! Today I just realized there is your review for this ticket. I couldn't really remember but I think I got conversion warning if I did not downcast it. Something about I shouldn't not have operation involving ssize_t and int in one statement. Strangely when I tested it again, it did not complain. Perhaps I used different distro back then.

Well, since Larry has a patch intersecting with this ticket, I guess we should let him does this work. Thanks, Serhiy!

I tried to reply you in the rietveld but I got an error page.

I don't have a patch for this issue.  If you're thinking of my "nullable ints" patch, that was just an experiment.  I'd prefer we figure out what we're going to do on this issue, and we can talk abut conversion to Argument Clinic later.

Here is the updated patch. I try to be conservative here, preserving the current behaviour.

Negative times is same as zero.
Default value (which can be passed by omitting times) means unlimited.

I did not change the signature "repeat(object [,times])" because the only suitable default value, in my opinion, is "repeat(object [,times=<unlimited>])" which is impossible at the moment.

Well, let see what Raymond has anything to say about this.

def repeat(object, times=-1):  => it could break the code in the wild.

Current behaviour: repeat(object) -> unlimited, repeat(object, -1) -> 0 repetitions.

Your patch does not address my concern.

My concern is that itertools.repeat doesn't parse its arguments like other Python functions.  It behaves differently depending on whether "times" is passed by position or by keyword.  Therefore its actual calling signature cannot be represented accurately with an inspect.Signature object.

Let me state this precisely.  Currently, inspect.signature doesn't produce a Signature object for itertools.repeat.  But let's assume for the moment that it did, and that the default value for the "times" parameter was -1.  Then, for this code:

  sig = inspect.signature(itertools.repeat)
  a = itertools.repeat('a')
  b = itertools.repeat('a', sig.parameters['times'].default)
  c = itertools.repeat('a', times=sig.parameters['times'].default)

I'd expect the a, b, and c objects to behave identically and be interchangeable.  Passing in the default value for an optional parameter should always produce the same result as not passing in a value for that parameter, and for positional-or-keyword parameters it shouldn't matter whether that's done by position or by keyword.

However, b is different from a and c: a and c yields infinitely-many 'a's, whereas b never yields anything.

I want to see a patch where, after applying the patch, a b and c would be interchangeable.  Such a patch should be *simpler* than the existing code, as it would remove all the special-case code that examines the length of the args tuple.

"Special cases aren't special enough to break the rules."  I think itertools.repeat's argument parsing should stop breaking the rules.

Larry said, "It behaves differently depending on whether "times" is passed by position or by keyword."

And that is the bug. It should be the same no matter whether we send times through positional or keyword. As Raymond has said earlier in this thread, "The preferred behavior is that a negative number always means 0."

So negative times through keyword must mean zero repetitions.

So it means we cannot set -1 as a default value for times because it will upset many people.

I am not sure at the moment in itertools.repeat,

  sig = inspect.signature(itertools.repeat)
  a = itertools.repeat('a')
  b = itertools.repeat('a', sig.parameters['times'].default)
  c = itertools.repeat('a', times=sig.parameters['times'].default)

a & b & c can and should be interchangebly. b & c, yeah, should be interchangeably. But not a. a is a special case. Of course, if we were to design this function from scratch again, we should make a & b & c interchangeably. That is the ideal case. We should be able to send default value explicitly and manually. But we are in beta phase. Can we alter the behaviour in this phase?

But if we have to make a & b & c interchangeably in Python 3.4 (for philosophical or pragmatic reason), I think None is sensible default value, and your nullable-int patch works in this case. And I think we need to get approval from Raymond Hettinger.

Vajrasky's last patch (v3) in general LGTM. I'm not sure about documentation changes, sample Python implementation is only a demonstration, it shouldn't be exact equivalent.

The logic of the patch is hard to follow.  However, it's still examining the size of args, and now it's examining the size of kwargs, and behaving differently based on these sizes is exactly what I don't want.

I've attached an example patch of how I would change itertools.repeat if I were doing it today, without the benefit of Argument Clinic.  If that approach seems reasonable, maybe we can redo it with Argument Clinic and get a signature too.

msg209063 from Larry states "My concern is that itertools.repeat doesn't parse its arguments like other Python functions."  From my viewpoint the only long term option is to bring repeat (and other functions that you may have found exhibiting this behaviour during the AC process) into line with other Python functions.  If that means a deprecation period and not being able to use AC in the short term so be it, or have I again misunderstood the technical aspects that you've already discussed, in which case you can all give me a good kicking? :)

I think you've understood it.

The problem is, in order to deprecate the behavior, we first must provide the new behavior.  (That's official policy, in PEP 5.)  It's untenable to say "you'll have to stop using 'times=-1' in the future, but you can't use 'times=None' yet".  And right now I'm pretty sure we shouldn't add "times=None" for 3.4.  Therefore, we can't even deprecate the old behavior yet.

From a user perspective the docs say this "itertools.repeat(object[, times]) - Make an iterator that returns object over and over again. Runs indefinitely unless the times argument is specified."  So to me the use of "Times=None" in the "equivalent to" section is simply a red herring, as the user often won't bother reading this and certainly won't see it from interactive help.  As for negative times values I'd say leave it as is, although it someone was to suggest deprecating this behaviour and raising a ValueError instead I'd have no objections.

Larry said, "A proper fix for the "bug" will require changing the semantics of the function. 
It's inappropriate to do that in 2.7, 3.3, and (now that we're in beta) 3.4."

I think we can not have it all and need to be pragmatic in this ticket. While we can not fix the default value quirk in 2.7, 3.3, and 3.4, at least we can make the negative times consistent whether it is sent through positional or keyword.

Otherwise, we have to fix the doc:

repeat(object [,times])
create an iterator which returns the object for the specified number of times.  If not specified, returns the object endlessly.If times is negative and is sent via positional, it means zero repetitions. But if times is negative and is sent via keyword, it means endless repetition.

And it doesn't feel right, does it?

Raymond said "The preferred behavior is that a negative number always means 0."  My proposal honors that.

We certainly aren't changing this in 3.3 and 2.7, and as release manager for 3.4 I'm pretty sure we aren't changing it there.

If "equivalent to" code is not considered to be part of documentation, then the meaning of negative times should be documented.

How to handle this "problem" in Python maintenance releases (2.7, 3.3 and soon-to-be 3.4) is being discussed here: https://mail.python.org/pipermail/python-dev/2014-January/132101.html

In case, we are taking "backporting the full fix" road, here is the patch. I modified Larry's patch based on Serhiy's suggestion and bettered the error message, added test, and fixed the doc.

"Better error message" -> without my fix, repeat('a', times='cutecat') will throw "an integer is required". I make it as such it will throw "times must be integer or None."

For what it's worth: I figured out how this happened.  Maybe it's obvious to you, but this behavior baffled me until I went back and looked at the revision history.

In revision e260d6daf784, the argument parsing for itertools.repeat looks like this:

    Py_ssize_t cnt = -1;

    if (type == &repeat_type && !_PyArg_NoKeywords("repeat()", kwds))
        return NULL;

    if (!PyArg_ParseTuple(args, "O|n:repeat", &element, &cnt))
        return NULL;

    if (PyTuple_Size(args) == 2 && cnt < 0)
        cnt = 0;

In the subsequent revision, 3dbdbc5e6d85, it was changed to this:

    Py_ssize_t cnt = -1;

    if (!PyArg_ParseTupleAndKeywords(args, kwds, "O|n:repeat", kwargs,
                                     &element, &cnt))
        return NULL;

    if (PyTuple_Size(args) == 2 && cnt < 0)
        cnt = 0;

The original intent is now clear: only allow "cnt" to be -1 if it wasn't specified as an argument.  The author simply forgot that "times" could now be passed in as a keyword argument too.

What the author *probably* wanted was something like this:

    PyObject *times_keyword;

    ...

    times_keyword = PyDict_GetItemString(kwargs, "times");
    Py_XDECREF(times_keyword);
    if ((PyTuple_Size(args) == 2 || times_keyword) && cnt < 0)
        cnt = 0;

But I suggest it's far too late to change it to that now.

Here is the ultimate patch for this bug. The doc fix is based on Larry's writing: https://mail.python.org/pipermail/python-dev/2014-January/132156.html

I made sure the patch could be compiled by Sphinx and displayed nicely. I added test and comment in the code.

I'm inclined to apply Vajrasky Kok's third version (with minor cleanups).

The rule will be pretty much what Guido stated but without adding a special case for times=None (that could be an added enhancement at a later time if the need arose):  "If I had complete freedom in redefining the spec I would treat positional and keyword the same, interpret absent or None to mean "forever" and explicit negative integers to mean the same as zero, and make repr show a positional integer >= 0 if the repeat isn't None."

The if-absent-run-forever rule matches what the decade old positional-only API does and what the newer keyword form does as well.  It also matches what the documented rough equivalent code does.

The negative-times-means-zero rule matches the current positional-only api, it matches list.__mul__ and str.__mul__, and it matches the documented equivalent code.   However, it does change the meaning of the keyword argument when the value is negative (the part that conflicts with the positional API, was never intended, nor was promised in the docs).

Larry's false dilemmas aside, I think that takes care of the core issue that a negative value for a keyword times-argument does not have the same behavior as it would for a positional times-argument.

The use of "None" for an optional argument in the "equivalent" code is red herring.  As Serhiy says, the "sample Python implementation is only a demonstration, it shouldn't be exact equivalent."  If Larry still perceives this to be "wildly out of sync", it isn't hard to put in the usual "times=sentinel" setup in the same code, but that only adds a little precision at the expense of clarity (i.e. readers are more likely to be confused by the sentinel trick than by the conventional way of noting optional arguments with None).

Please clarify, what is my "false dilemma"?

> Please clarify

I was referring to your first post, "I see two possible choices here ... [changing the signature to times=-1 or to times=None]".  

There was another choice, make the code work as originally intended where omitting the times argument repeats indefinitely and where negative values are treated the same as zero.

New changeset dce9dbc8e892 by Raymond Hettinger in branch '3.4':
Issue #19145:  Fix handling of negative values for a "times" keyword argument to itertools.repeat()>
http://hg.python.org/cpython/rev/dce9dbc8e892

New changeset 85dc4684c83e by Raymond Hettinger in branch '2.7':
Issue #19145:  Fix handling of negative values for a "times" keyword argument to itertools.repeat()>
http://hg.python.org/cpython/rev/85dc4684c83e

The main thing for me isn't that the function and its documentation-pseudocode are in sync (though in the long run this is desirable).  What's important to me is that the function have a sensible, relevant signature in Python.  There was simply no way to express the "times argument behaves differently when passed in by position vs. by argument" semantics in a signature.

I agree the new implementation is an improvement.  But there's still no way to represent repeat's signature in Python.  This is because "times" is an optional argument without a valid default value.  It should always hold true that calling a function and explicitly passing in an optional argument's default value should behave identically to not specifying that argument.  But there's no value I can pass in to "times" that results in the same behavior as not passing in "times".  That's why I prefer the "times=None" approach.

At some point I expect to get "nullable ints" into Argument Clinic (see #20341 ).  Once that's in, I propose we convert itertools.repeat to work with Argument Clinic, as follows:
  * We use a nullable int for the "times" parameter.
  * The "times" parameter would have a default of None.
  * If times=None, repeat would repeat forever.
repeat would then have an accurate signature.

Raymond: does that sound reasonable?

> there's still no way to represent repeat's signature in Python
There is a way using *args and **kwds but that isn't any fun (just like range() or min() in that regard).

Right now, repeat() does what it is supposed to do.  It may currently be inconvenient for the argument clinic, but changing repeat() in way that no one currently needs smacks of having the tail wag the dog ;-)

What I would like to see in the future is better support for optional arguments in
PyArg_ParseTupleAndKeywords.  Right now, "O|n:repeat" precludes us from using None or some sentinel object to mean the same as the-argument-was-omitted.   To put a None default value in now, we would have to do tricks with "O|O" and then manually write the "|n" validation, type conversion, and associated error messages.

> There is a way using *args and **kwds but that isn't any fun

That's why, earlier, I said a "sensible" signature.  Every function *could* get the signature "(*args, **kwargs)" but this imparts no useful semantic information.


> What I would like to see in the future is better support
> for optional arguments in PyArg_ParseTupleAndKeyword

It sounds to me like you're proposing adding "nullable int" support to PyArg_ParseTuple*.  I'm not going to; I see Argument Clinic as the way forward, and I'm adding it there instead.

In general I'd rather see work go into AC than into PyArg_ParseTuple*.  I think PyArg_ParseTuple* is already too complicated, and using AC gives the function a signature for free.  My hope is to increase the value proposition of AC so much that everyone agrees with me and we deprecate (but don't remove!) PyArg_ParseTuple*. :D


> changing repeat() in way that no one currently needs smacks of having
> the tail wag the dog

I concede that nobody (probably) needs a workable default value for the times argument.  But I suggest that giving functions sensible signatures is a worthy goal in its own right, and that the "times=None" semantics will get us there in a reasonable, backwards-compatible way.

Raymond, thanks for committing my patch but my name was already put into ACKS before this commit.

$ grep -R Vajrasky Misc/ACKS 
Vajrasky Kok
Vajrasky Kok

New changeset 463f499ef591 by Raymond Hettinger in branch '3.4':
Issue #19145:  Remove duplicate ACKS entry
http://hg.python.org/cpython/rev/463f499ef591

New changeset 07eb04003839 by Raymond Hettinger in branch '2.7':
Issue #19145:  Remove duplicate ACKS entry
http://hg.python.org/cpython/rev/07eb04003839