Opened 3 months ago
Closed 3 weeks ago
#35682 closed Cleanup/optimization (fixed)
Clarify Base<FOO>View usage in docstrings.
Reported by: | Jesús Leganés-Combarro | Owned by: | YashRaj1506 |
---|---|---|---|
Component: | Generic views | Version: | 5.1 |
Severity: | Normal | Keywords: | |
Cc: | Jesús Leganés-Combarro, Clifford Gama | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
At https://github.com/django/django/blob/a57596e443ecb67140e1a9fc0f0e87446b2d0174/django/views/generic/list.py#L175, BaseListView.get()
method is calling to self.render_to_response(context)
method. Fact is, I have searched in all the BaseListView
ancestor classes (MultipleObjectMixin
, ContextMixin
and View
) and was not able to find any reference to the render_to_response()
method, both in the documentation or the code itself. The only place I have been able to find it implemented is in the TemplateResponseMixin
class, that's templates oriented, and it's only related to BaseListView
because both classes are ancestors of ListView
.
In its current state, using BaseListView
class without overriding the BaseListView.get()
method (that already implements a proper handler for HTTP GET
method, so overriding would not be needed) would crash because render_to_response()
method can't be found. One solution would be to add render_to_response()
as an abstract method, and add it on the documentation forcing to be implemented in a child class, but render_to_response()
feels to be too much related to templates, and was not able to find any other rendering related function in View
or other ancestor class of BaseListView
... Another maybe better solution would be to move BaseListView.get()
method to ListView.get()
, left ing BaseListView
empty the same way that ListView
currently is, or just with a get_context_data()
method to include the checking if list is empty like a validation, but that last ones would be backward incompatibles... Is there any other better solution for this?
Change History (20)
comment:1 by , 3 months ago
Easy pickings: | set |
---|---|
Summary: | `BaseListView` class calling non existing `render_to_response()` method → Clarify Base<FOO>View usage in docstrings. |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 3 months ago
Type: | Bug → Cleanup/optimization |
---|
follow-up: 6 comment:3 by , 3 months ago
Adding the "Using this base class requires subclassing to provide a response mixin" comment in the Docstring would be really nice, but in addition to that, I would make it more explicit by adding an implementation of render_to_response()
that raise a NotImplementedError
exception, that at least would provide a more specific message than a generic "object has no attribute" one.
Besides that, although render_to_response
is focused on templates, is it also a good name for non-template-based Views? For example in my use case I'm generating a dict object with a GeoJSON FeaturesCollection
object and later setting it as data of a JsonResponse
object, there are no templates involved at all.
comment:4 by , 3 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:5 by , 3 months ago
Cc: | added |
---|
follow-up: 7 comment:6 by , 3 months ago
Replying to Jesús Leganés-Combarro:
Adding the "Using this base class requires subclassing to provide a response mixin" comment in the Docstring would be really nice, but in addition to that, I would make it more explicit by adding an implementation of
render_to_response()
that raise aNotImplementedError
exception, that at least would provide a more specific message than a generic "object has no attribute" one.
Besides that, although
render_to_response
is focused on templates, is it also a good name for non-template-based Views? For example in my use case I'm generating a dict object with a GeoJSONFeaturesCollection
object and later setting it as data of aJsonResponse
object, there are no templates involved at all.
Hey i am trying to solve this issue, i wanted to project this solution, One way is that the ListView
is inheriting from two classes called MultipleObjectTemplateResponseMixin
and BaseListView
and MultipleObjectTemplateResponseMixin
inherits from TemplateResponseMixin
, which contains the function render_to_response
so eventually when BaseListView.get()
is used in ListView
the render_to_reponse
method is available there from MultipleObjectTemplateResponseMixin
and hence works with no problem, but if the same thing happens while using BaseListView
we will obviously get error because TemplateResponseMixin
is not inheritied. So one way is We can add a docstring that this class(BaseListView
) is meant to be subclassed by ListView
, so use ListView
and let BaseListView
be just a base class meant to be inherited (we will raise a error too) .Would like to know your opinion, accordingly i will make the pr.
follow-ups: 8 9 comment:7 by , 3 months ago
So one way is We can add a docstring that this class(
BaseListView
) is meant to be subclassed byListView
, so useListView
and letBaseListView
be just a base class meant to be inherited (we will raise a error too) . Would like to know your opinion, accordingly i will make the pr.
I don't think that's quite accurate. While it is true that ListView
subclasses BaseListView
, it does not follow that BaseListView
must always be subclassed by ListView
.
I think better is:
Using this base class requires subclassing to provide a response mixin.
which I copied from BaseUpdateView
.
Btw, you may want to look at other BaseFOOView
s to see how they handle that, and also if other abstract base views are also missing that in their docstring.
comment:8 by , 3 months ago
Replying to Clifford Gama:
So one way is We can add a docstring that this class(
BaseListView
) is meant to be subclassed byListView
, so useListView
and letBaseListView
be just a base class meant to be inherited (we will raise a error too) . Would like to know your opinion, accordingly i will make the pr.
I don't think that's quite accurate. While it is true that
ListView
subclassesBaseListView
, it does not follow thatBaseListView
must always be subclassed byListView
.
I think better is:
Using this base class requires subclassing to provide a response mixin.
which I copied from
BaseUpdateView
.
Btw, you may want to look at other
BaseFOOView
s to see how they handle that, and also if other abstract base views are also missing that in their docstring.
Okay sure, i will look through other BaseFOOView
s and will return to you with a final proposal. Thanks on the insights.
follow-up: 10 comment:9 by , 3 months ago
Replying to Clifford Gama:
So one way is We can add a docstring that this class(
BaseListView
) is meant to be subclassed byListView
, so useListView
and letBaseListView
be just a base class meant to be inherited (we will raise a error too) . Would like to know your opinion, accordingly i will make the pr.
I don't think that's quite accurate. While it is true that
ListView
subclassesBaseListView
, it does not follow thatBaseListView
must always be subclassed byListView
.
I think better is:
Using this base class requires subclassing to provide a response mixin.
which I copied from
BaseUpdateView
.
Btw, you may want to look at other
BaseFOOView
s to see how they handle that, and also if other abstract base views are also missing that in their docstring.
I checked the other Base<FOO>View
and yes as written in BaseUpdateView
inside docstring
Using this base class requires subclassing to provide a response mixin.
this makes totally sense and while i checked BaseDetailView
and BaseListView
adding this very line would make things clear (As BaseDetailView
is also using render_to_response from
TemplateResponseMixin`) so my final proposal would be to add this
Using this base class requires subclassing to provide a response mixin.
inside the docstring of BaseDetailView
and BaseListView
.
I have made the above mentioned changes, is there anything else you would like to suggest otherwise i find it fine now and will make the Pr soon.
comment:10 by , 3 months ago
I think that's okay for now. You'll get more suggestions as necessary from reviewers in the PR
follow-up: 12 comment:11 by , 3 months ago
In addition of the DocString, what about adding an implementation of render_to_response()
that raise NotImplementedError()
to make the error explicit in runtime?
follow-up: 13 comment:12 by , 3 months ago
Replying to Jesús Leganés-Combarro:
In addition of the DocString, what about adding an implementation of
render_to_response()
thatraise NotImplementedError()
to make the error explicit in runtime?
Sounds like a good idea, but I wasn't sure about it since no NotImplementedError()
are raised in any of the other BaseFOOView
s and most other Mixins. I think we will have to ask about this in the Django Internals Forum.
comment:13 by , 3 months ago
Replying to Clifford Gama:
Replying to Jesús Leganés-Combarro:
In addition of the DocString, what about adding an implementation of
render_to_response()
thatraise NotImplementedError()
to make the error explicit in runtime?
Sounds like a good idea, but I wasn't sure about it since no
NotImplementedError()
are raised in any of the otherBaseFOOView
s and most other Mixins. I think we will have to ask about this in the Django Internals Forum.
I would personally add it in all the BaseFOOView
s, it's a bit nasty to call a method that doesn't exist just because it's expected to be implemented in a subclass.
OTOH, doing so would break compatibility in the cases where it's not implemented in a subclass, but in another sibling class in the MRO, so it would break in case the sibling class implementing it is defined after the BaseFOOView
s with the implementation raising the exception, but IMHO, that's already broken code that's sucesfully running just by luck they are defined in the correct MRO order.
comment:14 by , 3 months ago
Forum discussion for this issue: https://forum.djangoproject.com/t/notimplementederror-on-basefooview/33978/1
comment:15 by , 3 months ago
Patch needs improvement: | set |
---|
comment:17 by , 2 months ago
Has patch: | set |
---|
comment:18 by , 2 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:19 by , 2 months ago
Patch needs improvement: | unset |
---|
From reading the code, it looks like the
BaseListView
like the otherBase<FOO>View
s are meant to be subclassed.I tried creating a view inheriting the
BaseDetailView
, and I got the same error:
'MyBaseDetailView' object has no attribute 'render_to_response'
This line is missing from the docstring in
BaseListView
and theBaseDetailView
."Using this base class requires subclassing to provide a response mixin."
And I think that needs to be added. But you may have to check if other
Base<FOO>View
also need that documentation.