Fixed #30427, Fixed #16176 -- Corrected setting descriptor in Field.contribute_to_class().#14508
Fixed #30427, Fixed #16176 -- Corrected setting descriptor in Field.contribute_to_class().#14508carltongibson merged 2 commits intomainfrom
Conversation
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. |
3e1cb4d to
34937aa
Compare
|
I added a small note to the docs. |
08e376a to
be43470
Compare
There was a problem hiding this comment.
Okay fine by me, as long as the ticket exists :)
felixxm
left a comment
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 😅) |
be43470 to
ba10618
Compare
…ontribute_to_class(). Co-authored-by: Jarek Glowacki <jarekwg@gmail.com>
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 5fc4884ba104c502e9aac9b72c2dfec1d7df1560 — 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. 😜