Re: #25897 - managers defined on non-abstract base classes inherited by child classes

[Shai Berger]

On Sunday 07 February 2016 19:47:48 Alex Poleha wrote:
> I apply my condition to default managers indeed and I see no problem here.
> Default manager is just first manager defined on class(or on it's non
> concrete base). It has no additional magic.
> 

Well, I was using wrong terminology, sorry; when I said "default manager" I 
meant the automatic manager supplied by Django when there is no other. Sorry 
about being unclear.

> About your example. I would never have suggested such a thing. And there
> will be no need to redeclare 'objects' manager in Comment class for you
> example after applying my patch.
> 

That's all I wanted to be sure of; your example made me think otherwise, and I 
didn't look at the actual code.

Thanks,
Shai.

Re: #25897 - managers defined on non-abstract base classes inherited by child classes

[Alex Poleha]

I apply my condition to default managers indeed and I see no problem here. 
Default manager is just first manager defined on class(or on it's non 
concrete base). It has no additional magic.

About your example. I would never have suggested such a thing. And there 
will be no need to redeclare 'objects' manager in Comment class for you 
example after applying my patch.

Please look at my example.
For SimpleComment default manager is 'test_objects'. It doesn't have 
'objects' attribute.
For Comment before applying my patch there would be 'objects' attribute, 
but not in his own __dict__, but in Comment.__dict__, available through 
python mro. That's what I want to fix.

In my example if you want to have 'objects' attribute available for 
SimpleComment now and for Comment after applying my patch you will have to 
redeclare it explicitly, because these classes have 'test_objects' as 
default manager.


On Sunday, February 7, 2016 at 6:47:24 PM UTC+3, Shai Berger wrote:
>
> Reading your description again, it seems like you apply the condition to 
> default managers as well. Default managers are not "specific to the class 
> they 
> are defined on", and I see no problem in their inheritance. In particular: 
>
> class BaseComment(models.Model): 
> ... some fields, no manager 
>
>
> class Comment(BaseComment): 
> ... some fields, still no manager 
>
> Now, are you suggesting that Comment.objects must be defined explicitly? I 
> find 
> that odd. 
>
> Shai. 
>
> On Saturday 06 February 2016 18:07:22 Alex Poleha wrote: 
> > Thank you for the suggestion. Pull request is adjusted to give 
> deprecation 
> > warning instead of raising AttributeError. 
> > Yes, to silence the warning manager need to be be added to any subclass 
> > explicitly. It is explained in documentation 
> > <
> https://docs.djangoproject.com/en/dev/topics/db/managers/#custom-managers- 
> > and-model-inheritance> for ages, so I don't think there would be a 
> problem. 
> > 
> > среда, 3 февраля 2016 г., 19:18:24 UTC+3 пользователь Tim Graham 
> написал: 
> > > Could this go through a deprecation where any use of the inherited 
> > > managers to be removed will raise a warning for a couple releases? If 
> > > anyone is relying on the behavior, they just need to add the managers 
> to 
> > > any subclasses, correct? 
> > > 
> > > On Wednesday, February 3, 2016 at 9:16:41 AM UTC-5, Alex Poleha wrote: 
> > >> Hi. 
> > >> 
> > >> According to documentation 
> > >> <
> https://docs.djangoproject.com/en/1.9/topics/db/managers/#custom-manage 
> > >> rs-and-model-inheritance> managers defined on non-abstract base 
> classes 
> > >> are not inherited by child classes. In fact they're inherited via 
> > >> python MRO. I made pull request 
> > >>  to fix it. I find this 
> > >> 
> > >> inheritance embarrassing due to reasons, explained in documentation: 
> > >>1. Managers defined on non-abstract base classes are *not* 
> inherited 
> > >>by child classes. If you want to reuse a manager from a 
> non-abstract 
> > >>base, redeclare it explicitly on the child class. These sorts of 
> > >>managers are likely to be fairly specific to the class they are 
> > >>defined on, so inheriting them can often lead to unexpected 
> results 
> > >>(particularly as far as the default manager goes). Therefore, they 
> > >>aren’t passed onto child classes. 
> > >> 
> > >> I also think this example shows some reasons why this inheritance is 
> > >> embarrasing: 
> > >> 
> > >> class SimpleComment(models.Model): 
> > >> test_objects = models.Manager() 
> > >> 
> > >> class BaseComment(models.Model): 
> > >> pass 
> > >> 
> > >> class Comment(BaseComment): 
> > >> test_objects = models.Manager() 
> > >> 
> > >> print(hasattr(models.SimpleComment, 'objects')) #False 
> > >> print(hasattr(models.Comment, 'objects'))  #True, we may expect False 
> > >> here, since 'SimpleComment' gives False in similar sitation. 
> > >> print(models.Comment.objects.model) #, this 
> manager 
> > >> is not contributed to 'Comment' class 
> > >> 
> > >> Tim Graham suggests asking if anyone relying on this inheritance and 
> > >> documentation should be fixed instead. Any suggestions? 
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/792b29af-d2bb-4cfa-b93c-056d8c503e13%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: #25897 - managers defined on non-abstract base classes inherited by child classes

[Shai Berger]

Reading your description again, it seems like you apply the condition to 
default managers as well. Default managers are not "specific to the class they 
are defined on", and I see no problem in their inheritance. In particular:

class BaseComment(models.Model):
... some fields, no manager


class Comment(BaseComment):
... some fields, still no manager

Now, are you suggesting that Comment.objects must be defined explicitly? I find 
that odd.

Shai.

On Saturday 06 February 2016 18:07:22 Alex Poleha wrote:
> Thank you for the suggestion. Pull request is adjusted to give deprecation
> warning instead of raising AttributeError.
> Yes, to silence the warning manager need to be be added to any subclass
> explicitly. It is explained in documentation
>  and-model-inheritance> for ages, so I don't think there would be a problem.
> 
> среда, 3 февраля 2016 г., 19:18:24 UTC+3 пользователь Tim Graham написал:
> > Could this go through a deprecation where any use of the inherited
> > managers to be removed will raise a warning for a couple releases? If
> > anyone is relying on the behavior, they just need to add the managers to
> > any subclasses, correct?
> > 
> > On Wednesday, February 3, 2016 at 9:16:41 AM UTC-5, Alex Poleha wrote:
> >> Hi.
> >> 
> >> According to documentation
> >>  >> rs-and-model-inheritance> managers defined on non-abstract base classes
> >> are not inherited by child classes. In fact they're inherited via
> >> python MRO. I made pull request
> >>  to fix it. I find this
> >> 
> >> inheritance embarrassing due to reasons, explained in documentation:
> >>1. Managers defined on non-abstract base classes are *not* inherited
> >>by child classes. If you want to reuse a manager from a non-abstract
> >>base, redeclare it explicitly on the child class. These sorts of
> >>managers are likely to be fairly specific to the class they are
> >>defined on, so inheriting them can often lead to unexpected results
> >>(particularly as far as the default manager goes). Therefore, they
> >>aren’t passed onto child classes.
> >> 
> >> I also think this example shows some reasons why this inheritance is
> >> embarrasing:
> >> 
> >> class SimpleComment(models.Model):
> >> test_objects = models.Manager()
> >> 
> >> class BaseComment(models.Model):
> >> pass
> >> 
> >> class Comment(BaseComment):
> >> test_objects = models.Manager()
> >> 
> >> print(hasattr(models.SimpleComment, 'objects')) #False
> >> print(hasattr(models.Comment, 'objects'))  #True, we may expect False
> >> here, since 'SimpleComment' gives False in similar sitation.
> >> print(models.Comment.objects.model) #, this manager
> >> is not contributed to 'Comment' class
> >> 
> >> Tim Graham suggests asking if anyone relying on this inheritance and
> >> documentation should be fixed instead. Any suggestions?

Re: #25897 - managers defined on non-abstract base classes inherited by child classes

[Alex Poleha]

Thank you for the suggestion. Pull request is adjusted to give deprecation 
warning instead of raising AttributeError.
Yes, to silence the warning manager need to be be added to any subclass 
explicitly. It is explained in documentation 

 
for ages, so I don't think there would be a problem.

среда, 3 февраля 2016 г., 19:18:24 UTC+3 пользователь Tim Graham написал:
>
> Could this go through a deprecation where any use of the inherited 
> managers to be removed will raise a warning for a couple releases? If 
> anyone is relying on the behavior, they just need to add the managers to 
> any subclasses, correct?
>
> On Wednesday, February 3, 2016 at 9:16:41 AM UTC-5, Alex Poleha wrote:
>>
>> Hi. 
>>
>> According to documentation 
>> 
>>  managers 
>> defined on non-abstract base classes are not inherited by child classes. In 
>> fact they're inherited via python MRO. I made pull request 
>>  to fix it. I find this 
>> inheritance embarrassing due to reasons, explained in documentation:
>>
>>1. Managers defined on non-abstract base classes are *not* inherited 
>>by child classes. If you want to reuse a manager from a non-abstract 
>> base, 
>>redeclare it explicitly on the child class. These sorts of managers are 
>>likely to be fairly specific to the class they are defined on, so 
>>inheriting them can often lead to unexpected results (particularly as far 
>>as the default manager goes). Therefore, they aren’t passed onto child 
>>classes.
>>
>> I also think this example shows some reasons why this inheritance is 
>> embarrasing:
>>
>> class SimpleComment(models.Model):
>> test_objects = models.Manager()
>>
>>
>> class BaseComment(models.Model):
>> pass
>>
>>
>> class Comment(BaseComment):
>> test_objects = models.Manager()
>>
>> print(hasattr(models.SimpleComment, 'objects')) #False
>> print(hasattr(models.Comment, 'objects'))  #True, we may expect False 
>> here, since 'SimpleComment' gives False in similar sitation.
>> print(models.Comment.objects.model) #, this manager 
>> is not contributed to 'Comment' class
>>
>> Tim Graham suggests asking if anyone relying on this inheritance and 
>> documentation should be fixed instead. Any suggestions?
>>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/c26100b6-2589-473b-a47e-88a636dcee12%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: #25897 - managers defined on non-abstract base classes inherited by child classes

[Tim Graham]

Could this go through a deprecation where any use of the inherited managers 
to be removed will raise a warning for a couple releases? If anyone is 
relying on the behavior, they just need to add the managers to any 
subclasses, correct?

On Wednesday, February 3, 2016 at 9:16:41 AM UTC-5, Alex Poleha wrote:
>
> Hi. 
>
> According to documentation 
> 
>  managers 
> defined on non-abstract base classes are not inherited by child classes. In 
> fact they're inherited via python MRO. I made pull request 
>  to fix it. I find this 
> inheritance embarrassing due to reasons, explained in documentation:
>
>1. Managers defined on non-abstract base classes are *not* inherited 
>by child classes. If you want to reuse a manager from a non-abstract base, 
>redeclare it explicitly on the child class. These sorts of managers are 
>likely to be fairly specific to the class they are defined on, so 
>inheriting them can often lead to unexpected results (particularly as far 
>as the default manager goes). Therefore, they aren’t passed onto child 
>classes.
>
> I also think this example shows some reasons why this inheritance is 
> embarrasing:
>
> class SimpleComment(models.Model):
> test_objects = models.Manager()
>
>
> class BaseComment(models.Model):
> pass
>
>
> class Comment(BaseComment):
> test_objects = models.Manager()
>
> print(hasattr(models.SimpleComment, 'objects')) #False
> print(hasattr(models.Comment, 'objects'))  #True, we may expect False 
> here, since 'SimpleComment' gives False in similar sitation.
> print(models.Comment.objects.model) #, this manager 
> is not contributed to 'Comment' class
>
> Tim Graham suggests asking if anyone relying on this inheritance and 
> documentation should be fixed instead. Any suggestions?
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/4b744e92-7549-49b5-9b9b-b6158e53bb78%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

#25897 - managers defined on non-abstract base classes inherited by child classes

[Alex Poleha]

Hi. 

According to documentation 

 managers 
defined on non-abstract base classes are not inherited by child classes. In 
fact they're inherited via python MRO. I made pull request 
 to fix it. I find this 
inheritance embarrassing due to reasons, explained in documentation:

   1. Managers defined on non-abstract base classes are *not* inherited by 
   child classes. If you want to reuse a manager from a non-abstract base, 
   redeclare it explicitly on the child class. These sorts of managers are 
   likely to be fairly specific to the class they are defined on, so 
   inheriting them can often lead to unexpected results (particularly as far 
   as the default manager goes). Therefore, they aren’t passed onto child 
   classes.

I also think this example shows some reasons why this inheritance is 
embarrasing:

class SimpleComment(models.Model):
test_objects = models.Manager()


class BaseComment(models.Model):
pass


class Comment(BaseComment):
test_objects = models.Manager()

print(hasattr(models.SimpleComment, 'objects')) #False
print(hasattr(models.Comment, 'objects'))  #True, we may expect False here, 
since 'SimpleComment' gives False in similar sitation.
print(models.Comment.objects.model) #, this manager is 
not contributed to 'Comment' class

Tim Graham suggests asking if anyone relying on this inheritance and 
documentation should be fixed instead. Any suggestions?

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/d0b0f36a-28eb-40a1-8124-2a888b810c87%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.