Author wolma
Recipients mark.dickinson, rhettinger, serhiy.storchaka, tim.peters, wolma
Date 2018-03-27.15:34:59
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <e22fe8f2-ae7b-315f-cd43-d452f3c21e1b@biologie.uni-freiburg.de>
In-reply-to <1522134277.84.0.467229070634.issue33144@psf.upfronthosting.co.za>
Content
Serhiy:

> I like the idea in general, but have comments about the implementation.
> 
> __init_subclass__ should take **kwargs and pass it to super().__init_subclass__(). type(cls.random) is not the same as type(self.random). I would use the condition `cls.random is _random.Random.random` instead, or check if the method is in cls.__dict__.
> 
> This will break the case when random or getrandbits methods are patched after class creation or per instance, but I think we have no need to support this.
> 

My bad, sorry, and thanks for catching all these issues!

You're absolutely right about the class type checks not being equivalent 
to the original ones at the instance level.
Actually, this is due to the fact that I first moved the checks out of 
_randbelow and into __init__ just as Raymond would have done and tested 
this, but then I realized that __init_subclass__ looked just like the 
right place and moved them again - this time without testing on derived 
classes again.
 From a quick experiment it looks like types.MethodDescriptorType would 
be the correct type to check cls.random against and types.FunctionType 
would need to be checked against cls.getrandbits, but that starts to 
look rather esoteric to me - so you are probably right that something 
with a cls.__dict__ check or the alternative suggestion of `cls.random 
is _random.Random.random` are better solutions, indeed.

> We could support also the following cases:
> 
> 1.
>      class Rand1(Random):
>          def random(self): ...
>          # _randbelow should use random()
> 
>      class Rand2(Rand1):
>          def getrandbits(self): ...
>          # _randbelow should use getrandbits()
>          # this is broken in the current patch
> 

Right, hadn't thought of this situation.

> 2.
>      class Rand1(Random):
>          def getrandbits(self): ...
>          # _randbelow should use getrandbits()
> 
>      class Rand2(Rand1):
>          def random(self): ...
>          # _randbelow should use random()
>          # this is broken in the current code
> 

May be worth fixing, too.
History
Date User Action Args
2018-03-27 15:34:59wolmasetrecipients: + wolma, tim.peters, rhettinger, mark.dickinson, serhiy.storchaka
2018-03-27 15:34:59wolmalinkissue33144 messages
2018-03-27 15:34:59wolmacreate