Prevent raising exception when limit is 0 #4098
Conversation
| """ | ||
| if a == 0 or b == 0: |
There was a problem hiding this comment.
Not totally obv if we should do the check in here (less chance of missing the check elsewhere), or in the place where this is used (more semantically correct to the name of this func).
There was a problem hiding this comment.
@tomchristie this seems to just be used in get_html_context(). I can just check if self.limit is 0 there and take care of things without changing _divide_with_ceil.
There was a problem hiding this comment.
@tomchristie something along the lines of...
def get_html_context(self):
base_url = self.request.build_absolute_uri()
current = 1
final = 1
if self.limit > 0:
current = _divide_with_ceil(self.offset, self.limit) + 1
# The number of pages is a little bit fiddly.
# We need to sum both the number of pages from current offset to end
# plus the number of pages up to the current offset.
# When offset is not strictly divisible by the limit then we may
# end up introducing an extra page as an artifact.
final = (
_divide_with_ceil(self.count - self.offset, self.limit) +
_divide_with_ceil(self.offset, self.limit)
)
if final < 1:
final = 1
if current > final:
current = final
def page_number_to_url(page_number):
if page_number == 1:
return remove_query_param(base_url, self.offset_query_param)
else:
offset = self.offset + ((page_number - current) * self.limit)
return replace_query_param(base_url, self.offset_query_param, offset)
page_numbers = _get_displayed_page_numbers(current, final)
page_links = _get_page_links(page_numbers, current, page_number_to_url)
return {
'previous_url': self.get_previous_link(),
'next_url': self.get_next_link(),
'page_links': page_links
}There was a problem hiding this comment.
Yeah, something like that.
I'd probably do...
if self.limit:
... # determine current/final
else:
... # set default values for current/final
Current coverage is 91.41%@@ master #4098 diff @@
==========================================
Files 51 51
Lines 5473 5474 +1
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 5002 5004 +2
+ Misses 471 470 -1
Partials 0 0
|
|
@jpadilla Thanks for taking this on, and to drf for finally introducing coverage.io to me :) |
| ) | ||
| if final < 1: | ||
|
|
||
| if self.limit > 0: |
There was a problem hiding this comment.
I think I'd prefer just 'if self.limit' here
|
👍 Thanks folks. |
Description
This fixes #4079 which raised exception in the Browsable API when using
LimitOffsetPaginationandlimitis set to 0.Thanks to @awbacker for raising the issue.