#29735 closed Cleanup/optimization (wontfix)
MRO of DeleteView need to be changed.
Reported by: | seokhun kim | Owned by: | seokhun kim |
---|---|---|---|
Component: | Generic views | Version: | dev |
Severity: | Normal | Keywords: | deletemixin, basedeleteview, deleteview, generic view |
Cc: | Triage Stage: | Unreviewed | |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
(Please refer to Pull Request-10362-Updated inheritance chain of the generic DeleteView)
I think MRO of DeleteView need to be changed in order to clarify the inheritance hierarchy.
So I refactored BaseDeleteView and DeleteMixin.
- (1) What is better as the super class of BaseDeleteView ?
- Currently BaseDeleteView inherits from BaseDetailView.
- I think SingleObjectMixin is sufficient.
- I think Inheritance of Mixin is better than generic view itself.
- So I deleted BaseDetailView in the super class of BaseDeleteView.
- In order to inherit SingleObjectMixin instead of BaseDetailView,
- BaseDeleteView inherits from DeletionMixin.
- DeletionMixin inherits from SingleObjectMixin.
- (2) Where is better on the location of get(), post(), delete() methods ?
- Currently DeleteMixin provides those methods.
- It is bad that Mixin class provides those methods, I think.
- So My change is that BaseDeleteView provides get(), post() and delete() methods.
Change History (7)
comment:1 by , 6 years ago
Description: | modified (diff) |
---|---|
Needs documentation: | set |
Owner: | changed from | to
Status: | new → assigned |
Version: | 2.1 → master |
comment:2 by , 6 years ago
Triage Stage: | Unreviewed → Accepted |
---|
follow-up: 4 comment:3 by , 6 years ago
Triage Stage: | Accepted → Unreviewed |
---|
Please don't mark you own tickets as Accepted, only someone else who believes the proposed changes make sense can accept the idea.
I haven't investigated this in details but I'm a bit worried about changing the MRO and moving the methods off DeleteMixin
for the sake of purity. Don't get me wrong, I think that these changes would make sense if we were to start from scratch but in this case I feel breakage risks outweigh the benefits.
See the ticket triaging documentation for more details.
comment:4 by , 6 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
Replying to Simon Charette:
I haven't investigated this in details but I'm a bit worried about changing the MRO and moving the methods off
DeleteMixin
for the sake of purity. Don't get me wrong, I think that these changes would make sense if we were to start from scratch but in this case I feel breakage risks outweigh the benefits.
This seems right. I think for the breaking-change we have to close this as wontfix
.
I'm not 100% on that. I've opened a django-developers thread asking for other opinions. Very happy to re-open if we think this is something we can do.
comment:5 by , 6 years ago
You are saying "breaking-change".
However in my humble opinion, the code of DeleteView is not so difficult.
To speak simply, my change is using SingleObjectMixin instead of BaseDetailView.
I hope you can draw better conclusion through django-developers.
Anyway thank you for your review.
comment:6 by , 6 years ago
You are saying "breaking-change".
The proposal removes methods from DeletionMixin
, so if I'm relying on that, it clearly is a breaking change.
We might be able to work around that: the point of opening the discussion on django-developers was to put it in front of a wider audience, to make that decision better/easier. If you'd like to advocate this change there that would be great.
comment:7 by , 6 years ago
I think you are good to open this ticket to django-developers.
My point is, I hope people don't be too serious to consider this ticket.
This is my first contribution and I don't have much experience.
So I think it's not good policy to advocate this. I believe you...~
Thank you, Carlton Gibson.
Documentation exists in the Pull Request-10362-Updated inheritance chain of the generic DeleteView.