[Django] #29735: MRO of DeleteView need to be changed.
Django
-------------------------------------+-------------------------------------
Reporter: seokhun | Owner: nobody
kim |
Type: | Status: new
Cleanup/optimization |
Component: Generic | Version: 2.1
views | Keywords: deletemixin,
Severity: Normal | basedeleteview, deleteview, generic
Triage Stage: | view
Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
(Please refer to Pull Request: MRO of DeleteView was changed)
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29735>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | kim
Cleanup/optimization | Status: assigned
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: deletemixin, | Triage Stage:
basedeleteview, deleteview, | Unreviewed
generic view |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => seokhun kim
* status: new => assigned
* version: 2.1 => master
* needs_docs: 0 => 1
Old description:
> (Please refer to Pull Request: MRO of DeleteView was changed)
>
> 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.
New description:
(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.
--
Comment:
Documentation exists in the Pull Request-10362-Updated inheritance chain
of the generic DeleteView.
--
Ticket URL: <https://code.djangoproject.com/ticket/29735#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | kim
Cleanup/optimization | Status: assigned
Component: Generic views | Version: master
Severity: Normal | Resolution:
basedeleteview, deleteview, |
generic view |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/29735#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | kim
Cleanup/optimization | Status: assigned
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: deletemixin, | Triage Stage:
generic view |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Unreviewed
Comment:
Please don't mark you own tickets as ''Accepted'', only someone else who
believes the changes are adequate can do it.
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 [https://docs.djangoproject.com/en/2.1/internals/contributing
/triaging-tickets/#accepted ticket triaging documentation] for more
details.
--
Ticket URL: <https://code.djangoproject.com/ticket/29735#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | kim
Component: Generic views | Version: master
Keywords: deletemixin, | Triage Stage:
basedeleteview, deleteview, | Unreviewed
generic view |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
Comment:
Replying to [comment:3 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 [https://groups.google.com/d/topic
/django-developers/R_pgZTMc3oA/discussion django-developers thread] asking
for other opinions. Very happy to re-open if we think this is something we
can do.
--
Ticket URL: <https://code.djangoproject.com/ticket/29735#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | kim
Cleanup/optimization | Status: closed
Component: Generic views | Version: master
Severity: Normal | Resolution: wontfix
Keywords: deletemixin, | Triage Stage:
basedeleteview, deleteview, | Unreviewed
generic view |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by seokhun kim):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29735#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | kim
Cleanup/optimization | Status: closed
Component: Generic views | Version: master
Severity: Normal | Resolution: wontfix
Keywords: deletemixin, | Triage Stage:
basedeleteview, deleteview, | Unreviewed
generic view |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29735#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | kim
Cleanup/optimization | Status: closed
Component: Generic views | Version: master
Severity: Normal | Resolution: wontfix
Keywords: deletemixin, | Triage Stage:
basedeleteview, deleteview, | Unreviewed
generic view |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by seokhun kim):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29735#comment:7>