documentcloud / underscore-contrib

The brass buckles on Underscore's utility belt
MIT License
621 stars 117 forks source link

Util.operators: allow variadic methods to work with arrays #180

Closed jacobpurcell closed 10 years ago

jacobpurcell commented 10 years ago

Variadic util operators, such as _.add(), can now work with argument arrays.

e.g. _.add([1,2,3]) === 6

joshuacc commented 10 years ago

@jacobpurcell I like this idea, but I agree with @megawac that we should avoid flattening the arguments.

I'm of the opinion that the operator functions should take either an array/array-like or a series of numbers, but not both.

jacobpurcell commented 10 years ago

@joshuacc, @megawac, If I avoid deep flatten then I've got a couple of choices to use here

I can use numbers = Array.prototype.concat.apply([], arguments), then both numbers and arrays can be used at the same time: _.add(1, 2, [3, 4, 5]) _.add(1, 2) _.add([3, 4, 5])

or

I can use numbers = _.isArray(arguments[0]) ? arguments[0] : arguments or similar, then numbers and arrays cannot be mixed: _.add(1, 2) _.add([3, 4, 5])

Not sure how much performance difference there is between the two, but Array.prototype.concat is a native method.

Personally, I think there's a benefit for being able to do _.add(1, 2, [3, 4, 5]).

Note: Array.prototype.concat.is essentially what a shallow flatten does

megawac commented 10 years ago

I don't see the benefit of being able to _.add(1, 2, [3, 4, 5]). It's trivial enough to do _.add([1, 2].concat([3, 4, 5])) and generally you'll either know the parameters you have or a data array that you got from somewhere else. I don't think its worth complicating the api and concat.apply isn't fast (we dropped it from underscore [and lodash]) for a reason.

Also I wouldn't use _.isArray(arguments[0]) as that would prohibit passing arguments objects or other array likes

joshuacc commented 10 years ago

@jacobpurcell In order to accommodate array-like objects, we'd probably want to do something like this:

var numbers = typeof arguments[0].length == "number" ? arguments[0] : arguments;

jacobpurcell commented 10 years ago

Ok, will do.

jacobpurcell commented 10 years ago

Should be good now.

joshuacc commented 10 years ago

@jacobpurcell Looking good. Mind updating the docs as well? There are instructions in CONTRIBUTING.md.

jacobpurcell commented 10 years ago

Ok.

jacobpurcell commented 10 years ago

I've updated the docs. I rebuilt the index.html, although I imagine you'll do it again when you merge.

joshuacc commented 10 years ago

@jacobpurcell If you could make the last change that @megawac suggested (naming the first parameter for use in the ternary), I think this will be good to merge.

jacobpurcell commented 10 years ago

I've refactored the common code for checking the arguments object, and avoided using arguments[0]. Let me know if you think it's ok.

joshuacc commented 10 years ago

@jacobpurcell Left one comment, but otherwise it looks good.

jacobpurcell commented 10 years ago

Should be ready now.

jacobpurcell commented 10 years ago

@joshuacc - hi, are you able to merge this?

joshuacc commented 10 years ago

@jacobpurcell Yes. Once I get out of the office I'll take another look and (probably) merge it. Sorry for the delay!

jacobpurcell commented 10 years ago

No probs, Thanks.

joshuacc commented 10 years ago

@jacobpurcell Merged. Thanks for the contribution!