[Django] #34758: Paginator.validate_number implementation has undocumented change in 4.2

8 views
Skip to first unread message

Django

unread,
Aug 2, 2023, 4:50:05 AM8/2/23
to django-...@googlegroups.com
#34758: Paginator.validate_number implementation has undocumented change in 4.2
-----------------------------------------+----------------------------
Reporter: ruidc | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Other) | Version: 4.2
Severity: Normal | Keywords: pagination
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+----------------------------
https://github.com/django/django/commit/9101adc8ba66556c98f3955138d72b3492a6a60c#commitcomment-123385537

This for me at least was an undesirable behavior change from 4.1
The previous behavior should either be reinstated or the change
documented.

If there is agreement on direction, happy to make the PR either way.

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

Django

unread,
Aug 2, 2023, 5:08:30 AM8/2/23
to django-...@googlegroups.com
#34758: Paginator.validate_number implementation has undocumented change in 4.2
-------------------------------+--------------------------------------
Reporter: ruidc | Owner: nobody
Type: Uncategorized | Status: closed

Component: Core (Other) | Version: 4.2
Severity: Normal | Resolution: invalid

Keywords: pagination | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Changes (by Mariusz Felisiak):

* status: new => closed
* resolution: => invalid


Comment:

In the current implementation `num_pages` cannot be less than 1, so this
branch was unused. This looks like an issue in your custom paginator.

--
Ticket URL: <https://code.djangoproject.com/ticket/34758#comment:1>

Django

unread,
Aug 2, 2023, 5:24:42 AM8/2/23
to django-...@googlegroups.com
#34758: Paginator.validate_number implementation has undocumented change in 4.2
-------------------------------+--------------------------------------
Reporter: ruidc | Owner: nobody
Type: Uncategorized | Status: closed
Component: Core (Other) | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: pagination | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by ruidc):

Replying to [comment:1 Mariusz Felisiak]:


> In the current implementation `num_pages` cannot be less than 1, so this
branch was unused. This looks like an issue in your custom paginator.

From the code in main, that doesn't seem to be the case:
```
@cached_property
def num_pages(self):
"""Return the total number of pages."""
if self.count == 0 and not self.allow_empty_first_page:
return 0
...
```
Even so, the behavior for this public function changed when a valid
integer of zero is passed. Is documentation of this change not in order at
least?

--
Ticket URL: <https://code.djangoproject.com/ticket/34758#comment:2>

Django

unread,
Aug 2, 2023, 5:36:22 AM8/2/23
to django-...@googlegroups.com
#34758: Paginator.validate_number implementation has undocumented change in 4.2
-------------------------------+--------------------------------------
Reporter: ruidc | Owner: nobody
Type: Uncategorized | Status: closed
Component: Core (Other) | Version: 4.2
Severity: Normal | Resolution: invalid
Keywords: pagination | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by Mariusz Felisiak):

Replying to [comment:2 ruidc]:


> Replying to [comment:1 Mariusz Felisiak]:
> > In the current implementation `num_pages` cannot be less than 1, so
this branch was unused. This looks like an issue in your custom paginator.
>
> From the code in main, that doesn't seem to be the case:
> {{{
> @cached_property
> def num_pages(self):
> """Return the total number of pages."""
> if self.count == 0 and not self.allow_empty_first_page:
> return 0
> ...
> }}}

It's not possible when `allow_empty_first_page` is `True`.

> Even so, the behavior for this public function changed when a valid
integer of zero is passed.

I don't agree, this branch was unreachable.

--
Ticket URL: <https://code.djangoproject.com/ticket/34758#comment:3>

Reply all
Reply to author
Forward
0 new messages