Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstract iterator binding to createIterator and callbacks to createCallback #1582

Merged
merged 5 commits into from
May 19, 2014

Conversation

megawac
Copy link
Collaborator

@megawac megawac commented Apr 15, 2014

Highlights

There are many side effects of this commit and so I can go grab dinner I'm going to pull examples from lodash docs (see included tests for more examples):

var characters = [
  { 'name': 'barney', 'age': 36, 'blocked': false },
  { 'name': 'fred',   'age': 40, 'blocked': true }
];

// using "_.pluck" callback shorthand
_.map(characters, 'name');
// → ['barney', 'fred']

// using "_.pluck" callback shorthand
_.filter(characters, 'blocked');
// → [{ 'name': 'fred', 'age': 40, 'blocked': true }]

// using "_.where" callback shorthand
_.filter(characters, { 'age': 36 });
// → [{ 'name': 'barney', 'age': 36, 'blocked': false }]

_.min(characters, 'age');
// → { 'name': 'barney', 'age': 36, 'blocked': false };

// using "_.where" callback shorthand
_.find(characters, { 'age': 40 });
// →  { 'name': 'fred',   'age': 40, 'blocked': true }

// using "_.where" callback shorthand
_.reject(characters, { 'age': 36 });
// → [{ 'name': 'fred', 'age': 40, 'blocked': true }]

@jashkenas
Copy link
Owner

@braddunbar -- When you have a free moment, I'd be curious to hear what you think of this patch...


//can be used like findwhere
var list = [{a: 1, b: 2}, {a: 2, b: 2}, {a: 1, b: 3}, {a: 1, b: 4}, {a: 2, b: 4}];
var result = _.find(list, {a: 1}, 'can be used as findWhere');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this message goes on the following line (not as the context argument to _.find).

@braddunbar
Copy link
Collaborator

After mulling it over, I'm for this one. I like the property sugar and while I'm not particularly fond of the Function#call optimization there is precedent for it in Backbone.Events. 👍


function test() {}
test.map = _.map;
deepEqual(_.where([_, {a: 1, b: 2}, _], test), [_, _], 'checks properties given function');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this test as functions are just another object as far as _.where is concerned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not, but its a gotcha I had in a previous commit (4512921) which is why I added a test for it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough! :)

@jdalton
Copy link
Contributor

jdalton commented Apr 25, 2014

As #1609 noted that _.min and _.max should get this too.

@megawac
Copy link
Collaborator Author

megawac commented Apr 28, 2014

Whoops, I forgot to add tests for max and min but they both will benefit from lookup iterator. I'll do that right quick thanks

@jashkenas
Copy link
Owner

Mind rebasing this so that it merges in cleanly?

megawac and others added 5 commits May 18, 2014 11:45
Long coming optimization discussed in 1525, 1561 and elsewhere.
Follows lodash's lead.
See included tests and lodash docs for some examples

LookupIterators maps string properties (see jashkenas#1557) and objects to
_.matches.

As a result several of the helper functions such as _.where, _.pluck,
_.findWhere are handled by their parent implementations.
As discussed in PR with jdalton and @bradunbar
@megawac
Copy link
Collaborator Author

megawac commented May 18, 2014

@jashkenas rebase is done

jashkenas added a commit that referenced this pull request May 19, 2014
Abstract iterator binding to createIterator and callbacks to createCallback
@jashkenas jashkenas merged commit 6e24329 into jashkenas:master May 19, 2014
@megawac megawac deleted the Iterators2 branch May 20, 2014 19:51
@akre54 akre54 mentioned this pull request Aug 26, 2014
captbaritone added a commit to captbaritone/underscore that referenced this pull request Oct 4, 2016
I believe this test was added by accident by @megawac in jashkenas#1582
(specifically ca404d40f1705ffc84c5cd8a8a95a04373df2c76)

Given our current documentation, the behavior of this edge case is not
defined.

This test currently passes because arrays are technically objects and
thus `_.iteratee` treats `[]` as a `_.matches` matchers. `_.matches`
casts `[]` to `{}` via `_.extendOwn({}, []);` and returns a function
witch always returns `true`.

The fact that this test exists in `_.reject`'s test and not in
`_.iteratee`'s leads me to believe that this is not actually intended
behavior, but merely an implementation detail which has been
accidentally enforced in our tests.

I'm currently reworking my pull request jashkenas#2494 (deep property access) and
in a future where arrays are used for deep property access, `[]` will
return a function that always returns `undefined`, which seems like
a more reasonable behavior.

I thought it would be good to have this conversation separate from the
deep property matching pull request, but removing this test is
a prerequisite to implementing deep property access.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants