Ticket #216 (assigned defect)

Opened 3 years ago

Last modified 3 years ago

[PATCH] findAll returns empty array

Reported by: davelee Assigned to: Kaste (accepted)
Priority: minor Milestone: 1.0.1
Component: Active Record Keywords:
Cc:

Description (Last modified by Kaste)

Here is a patch to have findAll and equivalent return an empty array rather than false.

Arguments for the patch:

For calls to findAll and find('all',...), the calling code should eventually loop over at least some of the result. Forcing a calling convention of testing for false before looping adds verbosity and makes the code more error prone, not to mention tedious. A result of false forces the caller to test before going forward, a result of an empty array allows a choice of testing (via empty()), or not testing if it's unnecessary. This frees the calling code to be written cleaner and shorter.

Also, for reference, Rails returns an empty array for find_all and find(:all) calls that return no results.

Attachments

findAll-empty-array.patch (1.2 kB) - added by davelee on 09/09/08 14:51:33.

Change History

09/09/08 12:56:48 changed by Kaste

  • description changed.

+1

any side effects, we should care about?

regarding the patch: AFAIR $result is already an empty array() (findBySql returns always an array, so does findWithAssoc..()). So we could just return the $result without testing anything inside findEvery().

Proposed API is: find('first',..) or find(:id) returns either the object or false; find(all,..) returns always an array(). right?

09/09/08 14:50:30 changed by davelee

Side effects: I ran the Akelos unit tests, of which 101 failed both before and after the change. The few calls to _findEvery are all contained within AkActiveRecord, and all should continue to work with this patch. The main side effects would be the calling code that currently explicitly tests for false.

I've uploaded a new patch that just returns $result, because as you pointed out, no test is needed since findBySql and findWithAssociation both return empty arrays for empty result sets.

Yes, that is the proposed API change.

09/09/08 14:51:33 changed by davelee

  • attachment findAll-empty-array.patch added.

09/10/08 04:54:57 changed by Kaste

  • status changed from new to assigned.

Applied the patch (locally). All tests pass.

Since sintags uses empty(:object) when f.i. parsing {loop :collection} this API change should be unproblematic.

09/10/08 04:56:18 changed by Kaste

  • owner set to Kaste.
  • status changed from assigned to new.
  • description changed.

09/10/08 04:56:37 changed by Kaste

  • status changed from new to assigned.

09/10/08 11:59:27 changed by bermi

  • milestone changed from 0.9 to 1.0.

+1

Kaste, I'll move this to 1.0, as we don't want to break untested controllers which use the verbose way.