pop, join, and shift return result unless chaining. #1377

Closed
wants to merge 2 commits into from

5 participants

@braddunbar
Collaborator

I think we can all agree that returning the popped value from _(array).pop() is more useful than returning the array. This change keeps the existing behavior when chaining and returns the more useful value otherwise.

Note: This is a breaking change. We may want to put it off for some time before releasing.

@caseywebdev caseywebdev commented on the diff Dec 17, 2013
underscore.js
@@ -1266,7 +1266,7 @@
_.mixin(_);
// Add all mutator Array functions to the wrapper.
- each(['pop', 'push', 'reverse', 'shift', 'sort', 'splice', 'unshift'], function(name) {
+ each(['push', 'reverse', 'sort', 'splice', 'unshift'], function(name) {

push and unshift would need to be removed as well as they (annoyingly) return the length of the array instead of the array.

@braddunbar
Collaborator

That'd be fine, though I left them out for exactly this reason. Their return values aren't terribly useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jdalton

May be worth looking at what Lo-Dash did. By default it returns the unwrapped result values of pop, shift, and join, see here, and returns the wrapped result values when using chain().

Devs wanting to pop or shift values and still chain on the wrapped array may use _.tap:

// underscore usage
_(array).chain().tap(function(array) { array.pop(); }).map(...).value();

This way it's possible to support both behaviors chaining or not :D

@jashkenas jashkenas added the change label Feb 10, 2014
@jashkenas
Owner

@braddunbar — What do you think about @jdalton's choices on this?

@braddunbar
Collaborator

The change @jdalton mentions above is larger I think, but very similar. My goal here was to make _(...).pop() return something useful. I think removal of chaining support is a separate issue.

@braddunbar
Collaborator

Resolved merge conflicts above.

@jashkenas jashkenas added the frozen label Feb 10, 2014
@mlanza

The problem with pop is you're starting with an array and ending with an item (the last). pop mutates/splits the array at a given point, so what is most sensible to return: the array as the result of the side effect or the item removed as a side effect? Both of these can be returned now using existing underscore methods -- last or initial.

While it's nice in theory to keep all of the array methods, mutating methods don't seem to fit the idea of a functional library. So why give the appearance of mutation by using mutating function names? No matter what you decide pop should return it's misleading and won't match everyone's sensibilities. We started with a sequence so the method should return the sequence less the removed item. Or what I'm after is the removed item. Both results are meaningful in different circumstances and that's why underscore rightly allows you to get either result now.

_.pop(seq) // did a mutation occur?

Why offer a method in chaining syntax that isn't offered directly? Seems inconsistent. Is the aim to make the library more like an array or more like a functional library?

What does pop offer that isn't offered here?

_.last(seq)
_.initial(seq)

All the same applies to shift.

_.first(seq)
_.rest(seq)

What might be useful for unshift and push is concat, where all arguments are taken as arrays.

 var stooges = [larry];
_.concat(stooges, moe, curly); //push
_.concat(moe, curly, stooges); //shift

This approach is functional through and through and makes no implication of mutation.

@davidchambers

Very well put, @mlanza.

While it's nice in theory to keep all of the array methods, mutating methods don't seem to fit the idea of a functional library. So why give the appearance of mutation by using mutating function names?

I couldn't agree more.

I like the idea of _.concat. Array.prototype.concat (misleadingly) suggests mutation: Why else would @BrendanEich have made it an instance method rather than an Array function? For clarity, I sometimes write…

[].concat(a, b, c)

rather than…

a.concat(b, c)

_.concat would be easier to work with than Array.prototype.concat, as one needn't worry about setting this. It would play nicely with _.chain and _.partial. I think I'd use it quite frequently.

@jashkenas
Owner

We started with a sequence so the method should return the sequence less the removed item. Or what I'm after is the removed item. Both results are meaningful in different circumstances and that's why underscore rightly allows you to get either result now.

Well argued.

@jashkenas jashkenas closed this Mar 3, 2014
@jashkenas jashkenas added wontfix and removed frozen labels Mar 3, 2014
@jdalton

Both results are meaningful in different circumstances and that's why underscore rightly allows you to get either result now.

But Underscore doesn't allow you to get either result now when chaining, if you use _(...).pop() currently in Underscore you will not be able to get the value.

@braddunbar's PR is a nice compromise while the solution I outlined above takes it further and allowing both getting the value via _(...).pop() and _(...).chain().pop().value() and continuing to chain via _(...).chain().tap(function(array) { array.pop(); }).

@jashkenas jashkenas reopened this Mar 3, 2014
@mlanza

I'm suggesting that pop (and other mutating method names) should be dropped from underscore vernacular altogether. They don't belong. Call last or initial instead of pop.

_(...).last() // vs. _(...).pop()
_(...).initial() // for the sequence minus the "popped" item.

Something feels wrong about mutating partway into a chain of operations.

I think we can all agree that returning the popped value from _(array).pop() is more useful than returning the array. This change keeps the existing behavior when chaining and returns the more useful value otherwise.

Brad suggests that returning the popped value is more useful than returning the array. I've suggested that this is entirely subjective and that we arrive at two possible return values (over which to debate which is better to return) only because we are allowing for mutating methods at all. So eliminate them and the entire issue disappears, unless I'm missing something.

@davidchambers

I absolutely agree, @mlanza.

@jdalton

So eliminate them and the entire issue disappears, unless I'm missing something.

There's no need to remove API. Mutating arrays, esp pop/push are common enough and have clear expectations of behavior. If you don't like _(..).pop() then don't use it. Brad's patch is fine.

@mlanza

Removing the mutating methods makes choosing the right thing more obvious. We have pop and last but are they exactly the same or are there differences? Well, clearly there are differences since pop can only be called using chaining syntax and not directly (e.g. _.pop(...)).

Wouldn't it be nice if the direct api and the chain api were uniform? You wouldn't need that patchy bit of code that throws the mutating veneer on top of the library.

I'm not trying to make things difficult on Brad, just better for underscore. I guess I'm just struggling with finding a scenario where pop in a chain is necessary.

If pop absolutely must remain, why not just make it an alias of last? This would make it work exactly the way Brad wants it to work and keep uniform apis.

@jashkenas
Owner
  • We should keep the mutating methods. Don't use 'em if you don't want to use 'em.

  • The mutation methods should return the mutated array (not the added-or-removed-or-shifted-or-spliced value). If that's not currently the case, we should make it the case.

@jashkenas jashkenas closed this Mar 5, 2014
@jdalton

If pop absolutely must remain, why not just make it an alias of last? This would make it work exactly the way Brad wants it to work and keep uniform apis.

That would be really confusing. I've added other methods like _.remove and _.pull to my own implementation and aligned the existing chaining extensions, the step that Brad is trying to take, without incident.

@jdalton

@jashkenas

The mutation methods should return the mutated array (not the added-or-removed-or-shifted-or-spliced value). If that's not currently the case, we should make it the case.

There's no reason they should return the mutated array, the other outlined way allows both scenarios to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment