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 Clifford Gama, 3 months ago

Easy pickings: set
Summary: `BaseListView` class calling non existing `render_to_response()` methodClarify Base<FOO>View usage in docstrings.
Triage Stage: UnreviewedAccepted

From reading the code, it looks like the BaseListView like the other Base<FOO>Views 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 the BaseDetailView.

"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.

Last edited 3 months ago by Clifford Gama (previous) (diff)

comment:2 by Clifford Gama, 3 months ago

Type: BugCleanup/optimization

comment:3 by Jesús Leganés-Combarro, 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 YashRaj1506, 3 months ago

Owner: set to YashRaj1506
Status: newassigned

comment:5 by Clifford Gama, 3 months ago

Cc: Clifford Gama added

in reply to:  3 ; comment:6 by YashRaj1506, 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 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.

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.

in reply to:  6 ; comment:7 by Clifford Gama, 3 months ago

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.

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 BaseFOOViews to see how they handle that, and also if other abstract base views are also missing that in their docstring.

in reply to:  7 comment:8 by YashRaj1506, 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 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.

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 BaseFOOViews 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 BaseFOOViews and will return to you with a final proposal. Thanks on the insights.

in reply to:  7 ; comment:9 by YashRaj1506, 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 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.

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 BaseFOOViews 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.

Last edited 3 months ago by YashRaj1506 (previous) (diff)

in reply to:  9 comment:10 by Clifford Gama, 3 months ago

I think that's okay for now. You'll get more suggestions as necessary from reviewers in the PR

comment:11 by Jesús Leganés-Combarro, 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?

in reply to:  11 ; comment:12 by Clifford Gama, 3 months ago

Replying to Jesús Leganés-Combarro:

In addition of the DocString, what about adding an implementation of render_to_response() that raise 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 BaseFOOViews and most other Mixins. I think we will have to ask about this in the Django Internals Forum.

in reply to:  12 comment:13 by Jesús Leganés-Combarro, 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() that raise 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 BaseFOOViews 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 BaseFOOViews, 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 BaseFOOViews 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:15 by Clifford Gama, 3 months ago

Patch needs improvement: set

comment:16 by YashRaj1506, 2 months ago

Last edited 2 months ago by YashRaj1506 (previous) (diff)

comment:17 by YashRaj1506, 2 months ago

Has patch: set

comment:18 by Clifford Gama, 2 months ago

Triage Stage: AcceptedReady for checkin

comment:19 by YashRaj1506, 2 months ago

Patch needs improvement: unset

comment:20 by Sarah Boyce <42296566+sarahboyce@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In dc626fbe:

Fixed #35682 -- Updated docstrings for base view classes which require a response mixin.

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