Fixed #35682 -- Made Docstring updates in BaseDetailView and BaseList… #18493

YashRaj1506 · 2024-08-18T14:23:32Z

…View.

Trac ticket number

Branch description

Majority of the BaseView's like BaseUpdateView, BaseCreateView and other's has specified in their docstring and their docs that
"Using this base class requires subclassing to provide a response mixin." but in BaseListView and BaseDetailView where it is needed to , it is not mentioned inside the docstring but it is mentioned in their docs. So i added the same statement "Using this base class requires subclassing to provide a response mixin." inside the docstring of both BaseListView and BaseDetailView.

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.

…View.

github-actions

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

YashRaj1506 · 2024-08-18T17:01:07Z

@felixxm hey there, i am a new contributor here and this is my first PR, i wanted to ask why the two test cases are failing here. The two are Linters flake8 and black.

cliff688 · 2024-08-18T17:06:04Z

Hello @YashRaj1506!
You will find the details of the failures in your file changes

YashRaj1506 · 2024-08-18T17:21:36Z

Hello @YashRaj1506! You will find the details of the failures in your file changes

Thank you i got it now.

django/views/generic/list.py

django/views/generic/detail.py

Co-authored-by: Bhuvnesh <[email protected]>

cliff688

For completeness' sake, I suggest you go through the BaseViews in dates.py as well.

By the way, if a docstring already mentions that a BaseView is an abstract base class, it is reduntant to mention that it requires subclassing. In those cases I suggest: Requires a response mixin.

YashRaj1506 · 2024-08-20T12:44:54Z

For completeness' sake, I suggest you go through the BaseViews in dates.py as well.

By the way, if a docstring already mentions that a BaseView is an abstract base class, it is reduntant to mention that it requires subclassing. In those cases I suggest: Requires a response mixin.

Yeah i missed dates.py, i will commit the changes, and on the second thing you mentioned. UPDATE: I made the changes see if you want any other changes.

carltongibson

The wording should probably be consistent across the three cases.

cliff688

You left out many other Base Views in dates.py that neglect to mention their subclassing requirements in docstrings.

YashRaj1506 · 2024-08-20T18:29:59Z

You left out many other Base Views in dates.py that neglect to mention their subclassing requirements in docstrings.

oh my bad will go work over it, Will read the whole dates.py

YashRaj1506 · 2024-08-20T18:31:52Z

The wording should probably be consistent across the three cases.

Just to clarify, You mean to say the three places i made changes at, should have the same "this class is meant to be subclassed" message? As i added the response Mixin message in the third dates.py base class?

YashRaj1506 · 2024-08-21T07:06:45Z

You left out many other Base Views in dates.py that neglect to mention their subclassing requirements in docstrings.

For the docstrings which don't mention they are a abstract class like
"BaseMonthArchiveView", ""BaseDayArchiveView" and others, they just specify their functionalities of what it does. Should i add infront of it that "A abstract base class which does action-it-does. Requires a Response Mixin"? it will be enough ig.

cliff688 · 2024-08-22T16:20:11Z

For the docstrings which don't mention they are a abstract class like "BaseMonthArchiveView", ""BaseDayArchiveView" and others, they just specify their functionalities of what it does. Should i add infront of it that "A abstract base class which does action-it-does. Requires a Response Mixin"? it will be enough ig.

I think it's better if we use consistent wording for all Base Views with unclear docstrings. Like so:

class BaseMonthArchiveView(YearMixin, MonthMixin, BaseDateListView):
    """
    Base view for displaying a list of objects published in a given month.

    Requires a response mixin.
    """

Let me know what you think.

YashRaj1506 · 2024-08-22T16:28:23Z

For the docstrings which don't mention they are a abstract class like "BaseMonthArchiveView", ""BaseDayArchiveView" and others, they just specify their functionalities of what it does. Should i add infront of it that "A abstract base class which does action-it-does. Requires a Response Mixin"? it will be enough ig.

I think it's better if we use consistent wording for all Base Views with unclear docstrings. Like so:
class BaseMonthArchiveView(YearMixin, MonthMixin, BaseDateListView):
    """
    Base view for displaying a list of objects published in a given month.

    Requires a response mixin.
    """
Let me know what you think.

The edit that i made on the docstring (yet to commit) looks like this only, I think this would be good enough to go with. I will make the following changes and will tag you if i find something better.

As it was mentioned in suggestions, that the wording should be same everywhere for the base views, i made the changes accordingly.

fixed flake8 at line 654.

YashRaj1506 · 2024-08-27T13:13:32Z

@carltongibson @cliff688 I have made the changes accordingly, keeping both of your suggestions in mind. Will this commit be enough or do i need to make any other changes? Do let me know i will work on it.

cliff688

I think Using this base class requires subclassing to provide a response mixin. is wordier than Requires a response mixin.
What do you think.

cliff688 · 2024-08-27T15:35:52Z

django/views/generic/dates.py

@@ -625,8 651,10 @@ class TodayArchiveView(MultipleObjectTemplateResponseMixin, BaseTodayArchiveView

 class BaseDateDetailView(YearMixin, MonthMixin, DayMixin, DateMixin, BaseDetailView):
 """
- Detail view of a single object on a single date; this differs from the
- standard DetailView by accepting a year/month/day in the URL.
+ A base view for Detail view of a single object on a single date; this differs from


Suggested change

A base view for Detail view of a single object on a single date; this differs from

A base detail view for a single object on a single date; this differs from

the standard DetailView by accepting year/month/day in the URL.

cliff688 · 2024-08-27T15:43:10Z

django/views/generic/dates.py

@@ -300,7 300,11 @@ def _make_single_date_lookup(self, date):


 class BaseDateListView(MultipleObjectMixin, DateMixin, View):
- """Abstract base class for date-based views displaying a list of objects."""
+ """
+ A base view for date-based views displaying a list of objects.


The previous line was probably clearer and more concise:

Suggested change

A base view for date-based views displaying a list of objects.

Abstract base class for date-based views displaying a list of objects.

or maybe

Suggested change

A base view for date-based views displaying a list of objects.

Base class for date-based views displaying a list of objects.

YashRaj1506 · 2024-08-27T16:12:27Z

I think Using this base class requires subclassing to provide a response mixin. is wordier than Requires a response mixin. What do you think.

It was but as earlier @carltongibson quoted that all the base views should have a consistency in the wordings, i considered that and made the changes. I will make the minor sentence changes you recommended as those are important. Rest what do you think , Should we turn it back? eitherways it gives a more concrete message.

cliff688 · 2024-08-27T16:21:13Z

It was but as earlier @carltongibson quoted that all the base views should have a consistency in the wordings, i considered that and made the changes.

Indeed... I also agree that the wording should be consistent💯. Even so, there is still the question of which wording is better🤔. And this is what I meant for us to discuss...
Feel free to keep whichever one you prefer, but then the choice is better when it's consciously consisered, I think.

Fixed #35682 -- Made Docstring updates in BaseDetailView and BaseList…

b95b102

…View.

github-actions bot reviewed Aug 18, 2024

View reviewed changes

DevilsAutumn suggested changes Aug 19, 2024

View reviewed changes

django/views/generic/list.py Outdated Show resolved Hide resolved

django/views/generic/detail.py Outdated Show resolved Hide resolved

YashRaj1506 and others added 2 commits August 19, 2024 17:18

Update django/views/generic/list.py

c1ce8aa

Co-authored-by: Bhuvnesh <[email protected]>

Update django/views/generic/detail.py

4ff681c

Co-authored-by: Bhuvnesh <[email protected]>

cliff688 suggested changes Aug 20, 2024

View reviewed changes

YashRaj1506 added 2 commits August 20, 2024 18:24

Update dates.py

e4a66b6

Update dates.py

98399c7

carltongibson reviewed Aug 20, 2024

View reviewed changes

cliff688 reviewed Aug 20, 2024

View reviewed changes

YashRaj1506 added 3 commits August 27, 2024 17:15

Update dates.py

509de6c

As it was mentioned in suggestions, that the wording should be same everywhere for the base views, i made the changes accordingly.

Update dates.py

589935e

fixed flake8 at line 654.

Update dates.py

772c0ec

cliff688 reviewed Aug 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #35682 -- Made Docstring updates in BaseDetailView and BaseList… #18493

Fixed #35682 -- Made Docstring updates in BaseDetailView and BaseList… #18493

YashRaj1506 commented Aug 18, 2024 •

edited

Loading

github-actions bot left a comment

YashRaj1506 commented Aug 18, 2024 •

edited

Loading

cliff688 commented Aug 18, 2024 •

edited

Loading

YashRaj1506 commented Aug 18, 2024

cliff688 left a comment •

edited

Loading

YashRaj1506 commented Aug 20, 2024 •

edited

Loading

carltongibson left a comment

cliff688 left a comment

YashRaj1506 commented Aug 20, 2024

YashRaj1506 commented Aug 20, 2024

YashRaj1506 commented Aug 21, 2024

cliff688 commented Aug 22, 2024

YashRaj1506 commented Aug 22, 2024

YashRaj1506 commented Aug 27, 2024

cliff688 left a comment

cliff688 Aug 27, 2024

cliff688 Aug 27, 2024

YashRaj1506 commented Aug 27, 2024

cliff688 commented Aug 27, 2024

	A base view for Detail view of a single object on a single date; this differs from
	A base detail view for a single object on a single date; this differs from
	the standard DetailView by accepting year/month/day in the URL.

	A base view for date-based views displaying a list of objects.
	Abstract base class for date-based views displaying a list of objects.

	A base view for date-based views displaying a list of objects.
	Base class for date-based views displaying a list of objects.

Fixed #35682 -- Made Docstring updates in BaseDetailView and BaseList… #18493

Are you sure you want to change the base?

Fixed #35682 -- Made Docstring updates in BaseDetailView and BaseList… #18493

Conversation

YashRaj1506 commented Aug 18, 2024 • edited Loading

Trac ticket number

Branch description

Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

YashRaj1506 commented Aug 18, 2024 • edited Loading

cliff688 commented Aug 18, 2024 • edited Loading

YashRaj1506 commented Aug 18, 2024

cliff688 left a comment • edited Loading

Choose a reason for hiding this comment

YashRaj1506 commented Aug 20, 2024 • edited Loading

carltongibson left a comment

Choose a reason for hiding this comment

cliff688 left a comment

Choose a reason for hiding this comment

YashRaj1506 commented Aug 20, 2024

YashRaj1506 commented Aug 20, 2024

YashRaj1506 commented Aug 21, 2024

cliff688 commented Aug 22, 2024

YashRaj1506 commented Aug 22, 2024

YashRaj1506 commented Aug 27, 2024

cliff688 left a comment

Choose a reason for hiding this comment

cliff688 Aug 27, 2024

Choose a reason for hiding this comment

cliff688 Aug 27, 2024

Choose a reason for hiding this comment

YashRaj1506 commented Aug 27, 2024

cliff688 commented Aug 27, 2024

YashRaj1506 commented Aug 18, 2024 •

edited

Loading

YashRaj1506 commented Aug 18, 2024 •

edited

Loading

cliff688 commented Aug 18, 2024 •

edited

Loading

cliff688 left a comment •

edited

Loading

YashRaj1506 commented Aug 20, 2024 •

edited

Loading