| msg111554 - (view) |
Author: Łukasz Langa (lukasz.langa) *  |
Date: 2010-07-25 19:18 |
| Two behaviour problems with random.randrange: 1. Method argument `start` behaves as `stop` if `stop` is not defined: ====================================================================== >>> from random import randrange >>> help(randrange) Help on method randrange in module random: randrange(self, start, stop=None, step=1, int=<class 'int'>, default=None, maxwidth=9007199254740992) method of random.Random instance Choose a random item from range(start, stop[, step]). >>> randrange(10) #start=10 2 Clearly this is because randrange() mimicks the range() interface. Sphinx documentation specifies the behaviour clearly. The problem is, help() can mislead someone in this case. 2. `step` does not work when only `start` (acting as `stop`) specified: ======================================================================= >>> [randrange(0, 10, step=5) for i in range(10)] [5, 5, 5, 0, 5, 5, 5, 0, 0, 5] >>> [randrange(10, step=5) for i in range(10)] [5, 2, 4, 4, 6, 2, 7, 1, 5, 7] One would expect the latter to work the same way as the former. What next ========= Case 2 is clearly a bug that should be addressed. Raymond, Mark - would a patch for this be accepted for 2.7.x, 3.1.x and 3.2? If so I can provide one. In Case 1 we can do one of two things (or both): A. Tweak the docstring to specify the actual behaviour explicitly. B. Change the function definition to: `randrange(self, limit, *args, **kwargs)`. This is backwards compatible if we'd process `args` and `kwargs` correctly to keep the current interface intact (e.g. `start` would be processed in `kwargs`). This would leave a more predictable help(). Raymond, Mark - I'd say we absolutely do A. Does any of you object about B? |
|
|
| msg111557 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2010-07-25 21:04 |
| On issue 2: I agree that this is strange behavior, and would be interested to see a patch. It's not 100% clear to me what the patched code should do, though. In particular, an example like: >>> randrange(10, step=5) that mixes positional and keyword arguments is a bit odd. It's not clear in this case whether the '10' should be interpreted as a 'start' argument or a 'stop' argument: on a 'refuse the temptation to guess' basis, I'd say that 'randrange(10, step=5)' should be an error. More generally, perhaps any randrange call that mixes positional and keyword arguments should raise an exception? A related behaviour is: >>> randrange(start=10) 9 This should also be an error, IMO. Any patch should include comprehensive tests, of course. On the first issue, yes, it would be good to clarify the docs (see the range docs for how this might be done). [Łukasz] > B. Change the function definition to: `randrange(self, limit, *args, > **kwargs)`. This is backwards compatible if we'd process `args` and > `kwargs` correctly to keep the current interface intact (e.g. `start` > would be processed in `kwargs`). This would leave a more predictable > help(). Not sure I follow this bit: is this supposed to be a solution to problem 1 or problem 2? (Or both?) I don't really see the benefit of describing randrange this way in the docstring. |
|
|
| msg111558 - (view) |
Author: Łukasz Langa (lukasz.langa) * |
Date: 2010-07-25 21:31 |
| My suggestions on how the patches should work always assumed that we want 100% backward compatibility and there might exist some crazy code that is actually using positional/keyword argument mixing. If that is no problem for us then I'm all in favor of removing the posibility to mix positional with keyword args in this case. 1B is meant to solve the help() issue. The basic idea is to change the definition so that help() shows a sensible default for all cases. The current one when `start` becomes `stop` is not. The new definition would be: def randrange(self, limit, *args, **kwargs) The docstring would need to have an explanation on the role of `limit` in each sensible case. Then in the function body there would be some basic code going through what was specified by the user in the args, to assign these values to variables for existing implementation (which is OK). In effect, there would be 3 major advantages: - this is backward compatible - help() would not confuse users - invocations like randrange(start=30) or randrange(stop=10, step=8) would work properly If you don't strongly disagree then maybe it will be simpler to just provide a patch for you to review :) |
|
|
| msg111577 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2010-07-25 23:41 |
| I'm happy to improve the docs and docstring a bit to match the description for range(). I don't want to change any of the keyword arguments because it can break code that currently works and has been working for ages. It's a fact of life that the range() signature is complicated to express in pure Python. I don't feel any need to reformulate the existing code -- it has been working fine for people for a *very* long time. Conceptually, it's misleading but in practice people seem to just get it. |
|
|
| msg111580 - (view) |
Author: Łukasz Langa (lukasz.langa) * |
Date: 2010-07-25 23:48 |
| Raymond, I agree, that is reasonable. Would you like me to send a patch for the docstring or would you rather make the change on your own? |
|
|
| msg111584 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2010-07-26 00:08 |
| You are welcome to suggest wording. Aim for the shortest explanation that guides people to the right understanding. Being overly specific often causes more harm than good. Instead, try to be maximally helpful to someone just learning about randrange() for the first time and is trying to figure out what it does. |
|
|
| msg115739 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2010-09-07 04:48 |
| Added a doc fix to r84576. Advice is "don't do that" ;-) |
|
|