added remove method (issue #1856) #2906
Conversation
matinFT
changed the title
|
|
||
| // removes first element having the condition | ||
| export default function remove(collection, predicate){ | ||
| collection.splice(findIndex(collection, predicate), 1); |
There was a problem hiding this comment.
This is a start, but the intent of #1856 is to remove all elements that match the predicate. I think this will be easiest to do correctly if you go through collection backwards, i.e., remove the rightmost element first and the leftmost element last.
| @@ -583,4 +583,11 @@ | |||
| assert.deepEqual(_.chunk([10, 20, 30, 40, 50, 60, 70], 2), [[10, 20], [30, 40], [50, 60], [70]], 'chunk into parts of less then current array length elements'); | |||
| assert.deepEqual(_.chunk([10, 20, 30, 40, 50, 60, 70], 3), [[10, 20, 30], [40, 50, 60], [70]], 'chunk into parts of less then current array length elements'); | |||
| }); | |||
|
|
|||
| QUnit.test('remove', function(assert) { | |||
| assert.deepEqual(_.remove([1, 2, 3, 4], [1, 3, 4]), function(obj) {return(obj == 2)}); | |||
There was a problem hiding this comment.
This line seems to have been written with the following intuition: if you have the elements 1, 2, 3, 4 and you remove the elements 1, 3, 4, you'll be left with only element 2. While this intuition is right, this is not what you should be testing, and the way you're trying to test it also doesn't work.
You shouldn't be testing this, because the intent of _.remove is to take as its second argument a criterion (predicate) by which to decide whether an element should be removed, not a list of values to be removed. A function that accepts a list instead of a criterion as its second argument would be the modifying version of _.difference instead of the modifying version of _.reject. As-is (with your current implementation) the argument [1, 3, 4] will in fact be interpreted as a predicate, i.e., it will be passed to _.iteratee and be converted to a function equivalent to function(x) { return x[1][3][4]; }. I'm sure this is not at all what you meant; I think you meant function(x) { return x == 1 || x == 3 || x == 4; }.
The test also doesn't work, because assert.deepEqual() takes two arguments and then recursively compares them by value. You're passing it two arguments: the result of _.remove([1, 2, 3, 4], [1, 3, 4]) and a function. Unless you expect _.remove to return a function, this assertion is always going to fail. You should pass the expected value as the second argument to assert.deepEqual, not a function that performs a comparison.
There is also something missing. You should not only compare the result of _.remove to an expected value, but also verify that this result is the same array as the one that you passed as the first argument. After all, the intent of _.remove is that it modifies the array in place.
| assert.deepEqual(_.remove([{a: 1}, {a:3, b: 2}, 3], [{a: 1}, 3], function(obj) {return(obj.a == 3)})); | ||
| assert.deepEqual(_.remove([{a: {a: 1}}, 2, 3], [2, 3], function(obj) {return(obj.a.a == 1)})); | ||
| assert.deepEqual(_.remove([{a: 1, b: 2, c: 3}, {a:3, b: 2}, {c: 3}], [{a:3, b: 2}, {c: 3}], function(obj) {return(obj.b == 2 && obj.c == 3)})); |
There was a problem hiding this comment.
The next three tests have the same problems, but in addition, you mixed up the parentheses. You're now passing three arguments to _.remove and only one to assert.deepEqual.
There was a problem hiding this comment.
yeah sorry my bad. don't know how i came up with this! i messed up deepEqual and _. remove second arguments. i'll fix it and try with a new pr.
| @@ -1,3 +1,5 @@ | |||
| const { default: remove } = require("./modules/remove"); | |||
There was a problem hiding this comment.
This is not what you're supposed to be doing; please add your function to modules/index.js instead. You should only edit the sources in modules/, not the underscore.js or underscore-esm.js bundles.
From the fact that you were able to commit this, I can tell that you didn't run npm install, because otherwise the commit hook would have prevented it. Please run npm install before committing again.
No description provided.