Opened 18 years ago

Closed 16 years ago

#685 closed defect (fixed)

[patch] list_detail and archive_* generic views should default allow_empty to True

Reported by: django@… Owned by: nobody
Component: Generic views Version: dev
Severity: major Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Ideally default behaviour should correlate with the assumptions of the majority. Although it is clearly documented otherwise, I suspect that most people (like me :) will assume that the list_detail generic view handles an empty list exactly as it handles a populated list. I was surprised at getting 404s after reseting my database. Current bahviour has the potential to facilitate unnoticed bugs. I suggest defaulting 'allow_empty' to True. Of course this must be weighed against the hassle of backwards incompatibility and I suspect it may lose out on these grounds, but I thought it was worth ticketing for consideration.

Attachments (2)

allow_empty=True.diff (4.8 KB ) - added by Gary Wilson <gary.wilson@…> 17 years ago.
default value of allow_empty changed to True for the generic views (archive_* except archive_week which already defaulted to True and object_list) with the documentation changes as well.
685_allow_empty.diff (2.5 KB ) - added by mcroydon 16 years ago.
Per jacob's comments, apply allow_empty to generic index views and update docs. Apply patch with -p1.

Download all attachments as: .zip

Change History (30)

comment:1 by L.Plant.98@…, 18 years ago

I agree. I've already come across lots of scenarios where I have got it wrong and could easily get in wrong in the future and not notice. If you've already populated one item or more in the table you don't notice the behaviour with no items. When you later add searching facilities to the list, and someone enters a search with no results, they suddenly get a 404 error. The current behaviour is unintuitive, and is simply a shortcut for a common pattern that some apps might have, but as such it shouldn't be the default.

comment:2 by django@…, 18 years ago

Component: Admin interfaceGeneric views
Owner: changed from Adrian Holovaty to Jacob
priority: normalhigh
Severity: normalmajor

I'm glad this came back up. I can see this "feature" catching out newbies (and some oldbies) again and again. As the fix is backwards incompatible it should really be done before 1.0 -> Priority nudge.

comment:3 by Antti Kaihola, 18 years ago

It would help most newbies if at least the examples in the documentation used allow_empty=True for now.

comment:4 by real.human@…, 18 years ago

yep, i just spent many hours over two days trying to figure out why code that was previously working was now returning a "404" with no useful debug information, either. +1 for this change :)

comment:5 by Here, 18 years ago

Type: defect

comment:6 by anonymous, 18 years ago

Owner: changed from Jacob to anonymous
Status: newassigned
Type: defect

comment:7 by anjocee # gmail.com, 17 years ago

I agree, too.

It spent my two or three hours to find out why 404.
and lucky found the answer with google.

btw, I like djanjo very much.

comment:8 by Jared Kuolt, 17 years ago

I do NOT think this is a defect. It makes sense to me that when an object is not found, it throws a 404 (object not found).

comment:9 by anonymous, 17 years ago

milestone: Version 1.0
Resolution: duplicate
Status: assignedclosed
Version: 0.95

comment:10 by Gary Wilson <gary.wilson@…>, 17 years ago

Resolution: duplicate
Status: closedreopened
Version: 0.95SVN

Correcting probable spam.

And I also agree that the default should be allow_empty=True.

comment:11 by Gary Wilson <gary.wilson@…>, 17 years ago

Summary: For predictability list_detail generic view shoud default allow_empty to True[patch] For predictability list_detail generic view should default allow_empty to True

patch coming...

by Gary Wilson <gary.wilson@…>, 17 years ago

Attachment: allow_empty=True.diff added

default value of allow_empty changed to True for the generic views (archive_* except archive_week which already defaulted to True and object_list) with the documentation changes as well.

comment:12 by Gary Wilson <gary.wilson@…>, 17 years ago

Summary: [patch] For predictability list_detail generic view should default allow_empty to True[patch] list_detail and archive_* generic views should default allow_empty to True

comment:13 by Indy, 17 years ago

+1 from me. The current behaviour is a gotcha.

comment:14 by (none), 17 years ago

milestone: Version 1.0

Milestone Version 1.0 deleted

comment:15 by Gary Wilson <gary.wilson@…>, 17 years ago

Triage Stage: UnreviewedReady for checkin

From comments above, I would say this is accepted.

comment:16 by Malcolm Tredinnick, 17 years ago

Owner: changed from anonymous to Malcolm Tredinnick
Status: reopenednew
Triage Stage: Ready for checkinDesign decision needed

I'm not convinced this is "ready for checkin", since it breaks backwards compatibility. Four or five commenters in a ticket does not make a vast majority. At a minimum, I think this should wait until post-0.96 and be part of the backwards compatible changes for 1.0 if we include it.

I'm probably +1 on including the patch, but I know there is code out there relying on the current behaviour (I've written more than a few lines of such code) and breaking existing sites is not cool. So I'm moving this back to "design decision needed" in order to at least hold it over until the forthcoming release prior to the 1.0 push. Then we can decide (along with a bunch of other small items) whether we want to break backwards-compat.

comment:17 by Jeff Forcier, 17 years ago

I strongly agree that the default ought to be allow_empty=True, so make that five or six commenters ;)

I won't bother reiterating the arguments already made, but I will point out that if possible it'd be nice if this particular default setting was highlighted more strongly in the documentation or an FAQ. It's definitely a FAQ in the literal sense, as in I see it asked all the time.

comment:18 by Chris Beaven, 17 years ago

I'm +1 too.

comment:19 by cmgreen@…, 17 years ago

I just got bit by this issue. If it helps, generic views allows_empty brought me to this page wishing that this change is made. I troubleshot almost everything else about my config before I tracked this down.

comment:20 by Malcolm Tredinnick, 17 years ago

Will everybody please stop posting "me too" comments; it's adds no value to the ticket report, since a few extra comments in a userbase of many thousands does not change the weighting. As mentioned above, this is one of the backwards incompatibility changes we will have after 0.96 is released in preparation for 1.0. Not before 0.96.

comment:21 by Malcolm Tredinnick, 17 years ago

Err ... replace "backwards compatibility changes" with "backwards compatbility conversations" in the above. :(

comment:22 by vinay_sajip@…, 17 years ago

Backward compatibility can easily be maintained as follows: change the default value of allow_empty in the view definitions to a setting e.g. DEFAULT_ALLOW_EMPTY. This can be set to False by default in settings.py, and people who want to can change the setting to True on a per-instance basis. Doesn't this let us have our cake and eat it too?

comment:23 by Gary Wilson <gary.wilson@…>, 17 years ago

0.96 has been released now, do we want to go forward with this patch?

comment:24 by Malcolm Tredinnick, 17 years ago

It hasn't been forgotten, Gary, don't worry.

comment:25 by Jacob, 17 years ago

Patch needs improvement: set
Triage Stage: Design decision neededAccepted

Adrian and I feel that only the index views (archive_index, object_list) ought to default to allow_empty=True; I should get a 404 visiting (e.g.) a date that doesn't exist.

by mcroydon, 16 years ago

Attachment: 685_allow_empty.diff added

Per jacob's comments, apply allow_empty to generic index views and update docs. Apply patch with -p1.

comment:26 by mcroydon, 16 years ago

Patch needs improvement: unset

Removing patch needs improvement flag. Should be ready for checkin pending review. Should this be noted in BackwardsIncompatibleChanges?

comment:27 by Malcolm Tredinnick, 16 years ago

Triage Stage: AcceptedReady for checkin

Patch looks fine. Does need a note on the backwards-incompat page when it's committed.

comment:28 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

(In [6833]) Fixed #685 -- archive_index() and object_list() generic views allow empty
result sets by default now. Thanks, Gary Wilson and Matt Croydon.

This is a backwards incompatible change.

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