Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#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 seokhun kim)

(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 seokhun kim, 6 years ago

Description: modified (diff)
Needs documentation: set
Owner: changed from nobody to seokhun kim
Status: newassigned
Version: 2.1master

Documentation exists in the Pull Request-10362-Updated inheritance chain of the generic DeleteView.

comment:2 by seokhun kim, 6 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Simon Charette, 6 years ago

Triage Stage: AcceptedUnreviewed

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.

Last edited 6 years ago by Simon Charette (previous) (diff)

in reply to:  3 comment:4 by Carlton Gibson, 6 years ago

Resolution: wontfix
Status: assignedclosed

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 seokhun kim, 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 Carlton Gibson, 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 seokhun kim, 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.

Note: See TracTickets for help on using tickets.
Back to Top