search

Novactive / NovaCollection

Collection implementation that wraps regular PHP arrays.
MIT License
9 stars 3 forks source link

Refactor Collection::add() #3

Closed nozavroni closed 7 years ago

nozavroni commented 7 years ago

I see what you were trying to do with Collection:add() but making it capable of adding EITHER one item OR several items, is not going to work. At least not the way it's currently written. What if you want to add a traversable object?

// internally this will loop over the object and add its constituents to the collection, when the user's intention was most likely to simply add the object itself to the collection. 
$collection->add(new SomeTraversableObject);

If you want to add multiple items with one call, I suggest one of the following:

$collection->add($item1, $item2, $item3, $item4, ...$more);

// or... 

$collection->union($traversable_or_array);

UPDATE: I have left comments on the initial commit by @Plopix . It has this recommendation among others.

Plopix commented 7 years ago

Should be ok, I let you close if you agree ;)

nozavroni commented 7 years ago

Looks good. I will close this out as soon as I finish unit testing all of it. Thanks :)

Plopix commented 7 years ago

https://github.com/Novactive/NovaCollection/pull/11