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

Add toLookupIterator to _.every, _.some, _.map, _.filter #1561

Closed
jdalton opened this issue Apr 7, 2014 · 8 comments
Closed

Add toLookupIterator to _.every, _.some, _.map, _.filter #1561

jdalton opened this issue Apr 7, 2014 · 8 comments

Comments

@jdalton
Copy link
Contributor

jdalton commented Apr 7, 2014

Add toLookupIterator to _.every, _.some, _.map, _.filter.
By doing that you'd get something like _.pluck for free via _.map and get to avoid .call use when not needed.

@megawac
Copy link
Collaborator

megawac commented Apr 15, 2014

Edit: doesn't really address toLookupIterator which could be patched on...

I quickly wrote out two simple implementations that pass current specs. Plenty of room for improvement :)

Iterators2 is faster as its using .call() instead of .apply() however as a result it will mess up the iterator's arguments.length for everything but _.reduce. Therefore Iterator1 is more correct but can't see the side effects mattering.

Personally I'm impartial to the changes as it makes the source less elegant :/

@jashkenas
Copy link
Owner

If the iterator is only used internally, and the arguments.length are never used or exposed, then that sounds alright. Want to send over your Iterator2 branch as a PR?

@jdalton
Copy link
Contributor Author

jdalton commented Apr 15, 2014

The createIterator helper is a good thing, though it would be nice if it could address the issue too (incorporating or merging of toLookupIterator).

The problem with Iterator2 approach is that it passes 4 arguments to callbacks that expect 3 (_.each, _.filter, _.map, ...). I prefer Iiterator2 but it would be nice to have the argCount the callback expects.

@megawac
Copy link
Collaborator

megawac commented Apr 15, 2014

Is it worth adding in all the if statements required/using eval to handle the arity? Whose going to be writing

_.each([], function() {
   if(arguments.length === 3) { /* what? */ }
   else throw "you changed called with too many args yo"
})

And for sure, I'll add toLookupIterator when I'm ready to submit a PR

@megawac
Copy link
Collaborator

megawac commented Apr 15, 2014

If we decide the arguments length matters in Iterators2 it will need to handle the case for 1 (partition and sortedIndex), 3 and 4 arguments with its current uses

@jdalton
Copy link
Contributor Author

jdalton commented Apr 15, 2014

Is it worth adding in all the if statements required/using eval to handle the arity? Whose going to be writing

With composing being pretty popular I wouldn't want to introduce traps that might throw a wrench in someones callback code. You can use .apply or fork for specified argCount (could even default it to 3).

If we decide the arguments length matters in Iterators2 it will need to handle the case for 1 (partitionn and sortedIndex), 3 and 4 arguments with its current uses

Yap it may end up like this, though you might go with the .apply approach and work up to something like that later.

@megawac
Copy link
Collaborator

megawac commented Apr 15, 2014

Ya I was looking through your code last night and was tempted to just copy it. All right I'll follow your lead @jdalton. Should I add specs to verify arguments.length?

@jdalton
Copy link
Contributor Author

jdalton commented Apr 15, 2014

Should I add specs to verify arguments.length

Ya, I have that as a side effect of checking the arguments passed are correct.

megawac added a commit to megawac/underscore that referenced this issue Apr 15, 2014
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.
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

No branches or pull requests

3 participants