Re: [Django] #35682: Clarify Base<FOO>View usage in docstrings.

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner: (none)
Combarro |
Type: | Status: new
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Clifford Gama):

* type: Bug => Cleanup/optimization

--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner: (none)
Combarro |
Type: | Status: new
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:3>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:

Combarro | YashRaj1506
Type: | Status: assigned

Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by YashRaj1506):

* owner: (none) => YashRaj1506
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:4>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Clifford Gama):

* cc: Clifford Gama (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:5>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by YashRaj1506):

Replying to [comment:3 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:6>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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 `BaseFOOView`s to see how they handle
that, and also if other abstract base views are also missing that in their
docstring.
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:7>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by YashRaj1506):

Replying to [comment:7 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 `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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:8>

[Django]

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

should i make the above mentioned changes and make the pr?
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:9>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Clifford Gama):

I think that's okay for now. You'll get more suggestions as necessary from
reviewers in the PR
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:10>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:11>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Clifford Gama):

Replying to [comment:11 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 `BaseFOOView`s and
most other Mixins. I think we will have to ask about this in the Django
Internals Forum.
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:12>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jesús Leganés-Combarro):

Replying to [comment:12 Clifford Gama]:

> Replying to [comment:11 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 `BaseFOOView`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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:13>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Carlton Gibson):

Forum discussion for this issue: https://forum.djangoproject.com/t
/notimplementederror-on-basefooview/33978/1
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:14>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Clifford Gama):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:15>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by YashRaj1506):

PR has been made : https://github.com/django/django/pull/18493
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:16>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by YashRaj1506):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:17>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Clifford Gama):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:18>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506
Type: | Status: assigned
Cleanup/optimization |
Component: Generic views | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by YashRaj1506):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:19>

[Django]

#35682: Clarify Base<FOO>View usage in docstrings.
-------------------------------------+-------------------------------------
Reporter: Jesús Leganés- | Owner:
Combarro | YashRaj1506

Type: | Status: closed

Cleanup/optimization |
Component: Generic views | Version: 5.1

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"dc626fbe3ae0225b765df71d08fab02971dc6c6f" dc626fbe]:
{{{#!CommitTicketReference repository=""
revision="dc626fbe3ae0225b765df71d08fab02971dc6c6f"
Fixed #35682 -- Updated docstrings for base view classes which require a
response mixin.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35682#comment:20>