Manager-model reassignment on abstract subclassing
flo...@gmail.com
addresses the problem from the right angle, so if someone knows a
better way to go about fixing it, I'd like to open the discussion here
for better ways to fix this.
Essentially what's happening is that the manager is being inherited
properly from the abstract model, but each manager has a reference to
its parent model class, and that reference is not being updated. My
solution is to check in the descriptor if the calling model instance
matches the manager's reference, and if it doesn't, assign it. This
feels flimsy, as we probably shouldn't have to make this check at
all.
As far as I can tell, this can't be done in ModelBase.__new__, as the
reference is not yet set and therefore can't be checked.
Thoughts? Suggestions?
[1] http://code.djangoproject.com/ticket/7252
Thejaswi Puthraya
> I've opened up a ticket 7252 [1] with patch, but I'm not sure my patch
> addresses the problem from the right angle, so if someone knows a
> better way to go about fixing it, I'd like to open the discussion here
> for better ways to fix this.
code.djangoproject.com/ticket/7154). It has a patch that works but I
am not sure if it is the most elegant method.
> Essentially what's happening is that the manager is being inherited
> properly from the abstract model, but each manager has a reference to
> its parent model class, and that reference is not being updated. My
> solution is to check in the descriptor if the calling model instance
> matches the manager's reference, and if it doesn't, assign it. This
> feels flimsy, as we probably shouldn't have to make this check at
> all.
>
> As far as I can tell, this can't be done in ModelBase.__new__, as the
> reference is not yet set and therefore can't be checked.
>
> Thoughts? Suggestions?
>
> [1]http://code.djangoproject.com/ticket/7252
Cheers
Thejaswi Puthraya
http://thejaswi.info/
flo...@gmail.com
> code.djangoproject.com/ticket/7154). It has a patch that works but I
> am not sure if it is the most elegant method.
based on the summary that it was a different problem, because the
manager actually _does_ get copied over, but it just needs a slight
modification.
I do tend to like the implementation on that ticket better, as it gets
closer to the root of the problem. Are there any disagreements about
whether that patch should be applied?
-Eric Florenzano
Thejaswi Puthraya
complicated to understand) but I am using the patch for the
newcomments project. There are a couple of other quirks with Abstract
Models which I was assured were trivial and would be done with the nfa
merge.
Sebastian Noack
> > > code.djangoproject.com/ticket/7154). It has a patch that works
> > > but I am not sure if it is the most elegant method.
I am the author of #7154 and its patch. I thought first that it is
possible to fix this problem with less code if this is your
understanding of elegant. But I had to note that it isn't. ;-P
Look at #7252. The patch of this ticket adds only 2 lines. But it is
just an ugly hack. As Eric already noted it does not gets to the root
of the problem. The descriptor object's __get__ method is not the place
to handle such a case. The contibute_to_class method is the way to go.
But because of when contribute_to_class is called for the abstract base
model, we can not yet add the manager to the derived models. We have
to store the attribute name of the manager in the model options. So the
metaclass can handle this when deriving from an abstract model with
custom managers.
It think this is the most clean and reliable way to solve this problem.
But improvement purposes are welcome. ;)
> > Whoops! Sounds like we're talking about the same thing. I thought
> > based on the summary that it was a different problem, because the
> > manager actually _does_ get copied over, but it just needs a slight
> > modification.
As far as I know it was only present in derived models because of
derived models inherits all managers from the parent abstract model.
But it is no "copied" as done by _copy_to_model, which was the reason
for the behavior of inherited managers.
> > I do tend to like the implementation on that ticket better, as it
> > gets closer to the root of the problem. Are there any
> > disagreements about whether that patch should be applied?
Thanks. Although I wrote this patch, I have a disagreement. ;-P
Tests are still missing. But I think I can wrote some test cases at the
weekend or during next week.
Regards
Sebastian Noack
Sebastian Noack
myself, I have already uploaded a new version including test cases.
I found a bug by the way, that breaks the ability to derive from a not
abstract model, but I have fixed it. If you are already using the
previous version of my patch, you should update now.
I think the patch is ready to get merged into trunk now, but I can not
decide this. What does the maintainers think?
Regards
Sebastian Noack