Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

_.countBy and _.groupBy should only add values to own, not inherited, properties #736

Closed
jdalton opened this Issue Sep 2, 2012 · 5 comments

Comments

Projects
None yet
2 participants
Contributor

jdalton commented Sep 2, 2012

_.countBy and _.groupBy should only add values to own, not inherited, properties.

  _.groupBy = function(obj, val) {
    return group(obj, val, function(result, key, value) {
      (_.has(result, key) ? result[key] : (result[key] = [])).push(value);
    });
  };
Owner

jashkenas commented Sep 20, 2012

result in these cases is always a naked object. I don't think we need the extra check here.

@jashkenas jashkenas closed this Sep 20, 2012

Contributor

jdalton commented Sep 20, 2012

the check is to not collide with existing method names valueOf, toString, and so on because if the dev uses one of those it will error (as it will think it has an array on it, when it's a function).

Owner

jashkenas commented Sep 20, 2012

Ah, gotcha.

Owner

jashkenas commented Sep 20, 2012

Ah, wait -- I'm actually still not sure about this -- I don't see the use case where this change would be handy (grouping by the toString still wouldn't work properly. Got a failing test case that you think could be useful in an app?

Contributor

jdalton commented Sep 20, 2012

I don't see the use case where this change would be handy (grouping by the toString still wouldn't work properly.

Not sure what you mean. If you attempted to group by a property that exists on the plain objects prototype chain it will think it exists and so attempt to call .push.

A simple test case

// will throw an error
_.groupBy([4.2, 6.1, 6.4], function(num) {
  return Math.floor(num) > 4 ? 'hasOwnProperty' : 'constructor';
});

jashkenas added a commit that referenced this issue Sep 20, 2012

Fixes #736 -- allow groupBy and countBy to work with dynamic keys tha…
…t are the same as Object.prototype properties.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment