Conversation
jgonggrijp
left a comment
jgonggrijp
left a comment
There was a problem hiding this comment.
Very good @matinFT. Thanks for moving forward! We're nearly there.
I'm not seeing an update in underscore.js and underscore-esm.js. It's good that you didn't edit those files manually, but I expected the commit hook to update them automatically given your addition in modules/index.js. Did you not run npm install, or is the commit hook not working for you for some reason?
I see three remaining steps. You can continue adding and pushing new commits to your remove_method branch, they will be included in the current PR automatically (so you don't have to open a new PR).
- Get the commit hook to work so that
underscore.jsis automatically updated. - Process my comments below.
- Add documentation for
_.removein theindex.html.
Please let me know if you need any help. Good luck!
| @@ -0,0 +1,8 @@ | |||
| // removes first element having the condition | |||
There was a problem hiding this comment.
| // removes first element having the condition | |
| import cb from './_cb.js'; | |
| // removes first element having the condition |
This is required for my next suggestion...
| @@ -0,0 +1,8 @@ | |||
| // removes first element having the condition | |||
| export default function remove(collection, predicate) { | |||
| var index = collection.length; | |||
There was a problem hiding this comment.
... which is to pass the predicate through our internal cb function. This enables the iteratee shorthands, so users can for example do _.remove(anArray, { a: 1 }) (which would be equivalent to your testcase where you're passing function(obj) { return obj.a == 1; } as the second argument).
| var index = collection.length; | |
| var index = collection.length; | |
| predicate = cb(predicate); |
| export default function remove(collection, predicate) { | ||
| var index = collection.length; | ||
| while(index--){ | ||
| if(predicate(collection[index])) collection.splice(index, 1) |
There was a problem hiding this comment.
Following the conventions of other Underscore functions, it would be nice to also pass the index and the whole collection to the predicate.
| if(predicate(collection[index])) collection.splice(index, 1) | |
| if (predicate(collection[index], index, collection)) collection.splice(index, 1); |
| if(predicate(collection[index])) collection.splice(index, 1) | ||
| } | ||
| return collection; | ||
| } No newline at end of file |
There was a problem hiding this comment.
Please add a linebreak at the end of the file.
| var result = [1, 2, 3, 4] | ||
| _.remove(result, function(obj) {return(obj == 2)}) | ||
| assert.deepEqual(result, [1, 3, 4]); |
There was a problem hiding this comment.
This is good. I now understand that your tests in the previous PR where intended differently than I thought.
There is just one thing missing: these tests aren't verifying yet that result is also returned from _.remove. You can fix this as follows:
| var result = [1, 2, 3, 4] | |
| _.remove(result, function(obj) {return(obj == 2)}) | |
| assert.deepEqual(result, [1, 3, 4]); | |
| var result = [1, 2, 3, 4]; | |
| assert.strictEqual(_.remove(result, function(obj) {return(obj == 2)}), result); | |
| assert.deepEqual(result, [1, 3, 4]); |
|
Follow-up on #2906. |
3 participants
No description provided.