Model::all doesn't always return an array

[koenpunt]

As pointed out by @daniel-aranda in #385 (comment), Model::all doesn't always return an array.

For Model::find I find this acceptable, but all implies a collection.

@jpfuentes2 can you shed some light on this?

[jpfuentes2]

I think a failing test case would be easy to push for demonstration purposes. Interested, @daniel-aranda?

[jpfuentes2]

I read #385 and it definitely seems to be a bug for the case of Model::all(1). That should be a model instance and not an array.

[koenpunt]

You think Model::all should return a single model? Then what is the use of the $single = false; flag, which is set when find is called with 'all'

[jpfuentes2]

I think Model:all(1) for a single argument should return a model. What do you think?

[koenpunt]

That's already the case for ::find, so that would mean ::find and ::all are identical..?

[jpfuentes2]

Ruby's AR has all as an alias for find(:all).

Do you think it should always return an array? I'm not sure I know your position yet.

[koenpunt]

yes i think it should always be an array when invoked as Model::all.

Ruby AR doesn't even accept an integer as first parameter:

irb(main):017:0> Tracker.all(6)
NoMethodError: undefined method `assert_valid_keys' for 6:Fixnum

More results:

irb(main):016:0> Tracker.all({conditions: {id: 6}})
  Tracker Load (0.3ms)  SELECT `trackers`.* FROM `trackers` WHERE `trackers`.`id` = 6
=> [#<Tracker id: 6, title: "NU.nl", url: "http://www.nu.nl/algemeen/", selector: "div.excerpt", frequency: 5, user_id: 1, created_at: "2011-05-24 13:59:31", updated_at: "2012-04-18 20:00:19", status: 1>]

irb(main):014:0> Tracker.find(:all, 6)
  Tracker Load (0.3ms)  SELECT `trackers`.* FROM `trackers`
=> [#<Tracker id: 6, title: "NU.nl", url: "http://www.nu.nl/algemeen/", selector: "div.excerpt", frequency: 5, user_id: 1, created_at: "2011-05-24 13:59:31", updated_at: "2012-04-18 20:00:19", status: 1>, #<Tracker id: 7, title: "Koen", url: "http://www.koen.pt/", selector: "h2", frequency: 5, user_id: 1, created_at: "2011-05-25 00:02:04", updated_at: "2011-08-16 22:45:17", status: 1>, ....]

[jpfuentes2]

Okay, I think was doing too much multi-tasking and misunderstood this issue. We're on the same page now: all should always return an array. My only concern here is that this could break code where users erroneously depended on a single model return value.

Should this only go into the next version release? Speaking of which, @cvanschalkwijk requested that I change the default branch so that we start merging into a next-release-branch instead of master. Thoughts there?

[koenpunt]

You mean that users won't accidentally open PR against master? Sure, sounds good. The default branch would become 1.1-dev then?

[koenpunt]

I will open a PR against 1.1-dev with a solution for this issue somewhat later.

[daniel-aranda]

Thank You guys.