Fixed #30427, Fixed #16176 -- Corrected setting descriptor in Field.contribute_to_class(). #14508
Conversation
| # Override related field accessor. | ||
| Model.fk_id = property(lambda self: 'ERROR') |
There was a problem hiding this comment.
if you can't make this clash via @property any more, is the check still valid? Or at least could the error message and/or test model be changed?
There was a problem hiding this comment.
I've pushed a second commit removing this test.
Not 100% convinced...
We (@felixxm) discussed previously on the original PR. IIRC we said we'd leave it, and maybe adjust to add a check going the other way. (i.e. Cover I tried to define the pk_id property, but it got overridden vs the previous Because I'd defined the property the field descriptor wasn't set.)
I'd like to address that as a separate cleanup ticket.
- This has been going on too long already.
- And there are real issues being fixed here vs a system check/config error.
🤔
There was a problem hiding this comment.
Okay fine by me, as long as the ticket exists :)
There was a problem hiding this comment.
Created follow-up at ticket-32847
(Will leave this conversation unresolved so folks maybe see it as they're browsing.)
Yes definitely. Perhaps just a really terse description is enough... For anyone attempting diamond inheritance (certainly a small minority) this note may be enough (?). I don't think a long section with diagrams is warranted, given the rarity. |
carltongibson
force-pushed
the
pr/11337-redux
branch
2 times, most recently
from
3e1cb4d
to
34937aa
Compare
|
I added a small note to the docs. |
carltongibson
force-pushed
the
pr/11337-redux
branch
from
08e376a
to
be43470
Compare
| # Override related field accessor. | ||
| Model.fk_id = property(lambda self: 'ERROR') |
There was a problem hiding this comment.
Okay fine by me, as long as the ticket exists :)
There was a problem hiding this comment.
@carltongibson Thanks for pushing this to the finish line 🏁
@jarekwg Thanks for all your efforts 🥇
@adamchainz Thanks for reviews 🕵️
|
Thanks both. I'll tidy this up and pull it in. (What a set of tickets 😅) |
carltongibson
force-pushed
the
pr/11337-redux
branch
from
be43470
to
ba10618
Compare
…ontribute_to_class(). Co-authored-by: Jarek Glowacki <jarekwg@gmail.com>
carltongibson
force-pushed
the
pr/11337-redux
branch
from
ba10618
to
225d965
Compare
|
Will this be backported to 3.2.x? |
|
No, the backport policy only allows “Security fixes and data loss bugs” for a feature change like this: https://docs.djangoproject.com/en/dev/internals/release-process/#supported-versions . |
|
Ok, thanks! |
Replaces #11337.
Updates, and puts the tests in place that were hanging from the previous PR. It's difficult to follow so I've commented hoping to make them clear (for now and the next poor soul coming back to this 😀)
We had release notes in an earlier version 5fc4884 — those were incorrect since the multiple inheritance example never worked. See 4bbe826.
The change in behaviour proposed is allowing that structure. See the adjustment in the (now named)
test_multiple_inheritance_allows_inherited_fieldmethod.Inheritance works ≈as expected except for the depth-first resolution in diamond-shaped cases. Calling that out in
docs/topics/db/models.txtis I think probably worth while. I need to think about exactly where to put that/what to say. Input welcome. 😜