Added CKEDITOR.tools.array.find #2705
Conversation
core/tools.js
Outdated
| * | ||
| * @param {Array} array An array to be iterated over. | ||
| * @param {Function} fn A function with the signature `(a, index, array) -> b`. | ||
| * @param [thisArg=undefined] The context object for `fn`. |
core/tools.js
Outdated
| @@ -1950,6 +1950,40 @@ | |||
| return ret; | |||
| }, | |||
|
|
|||
| /** | |||
| * Returns the first element in the array for which the given callback `fn` returns true. | |||
There was a problem hiding this comment.
true might be wrapped in backticks, just like undefined is a line below.
core/tools.js
Outdated
| * var array = [ 'foo', 'bar', 'baz', 'faz' ]; | ||
| * | ||
| * CKEDITOR.tools.array.find( array, function( item ) { | ||
| * return item.indexOf( 'az' ) > -1; |
There was a problem hiding this comment.
This sample is pretty nice, but uses nasty indexOf, I'd suggest using even simpler example, with an array of numbers. And find should simply look for a value that is higher than some number.
That would be even simpler than this example.
core/tools.js
Outdated
| * ``` | ||
| * | ||
| * @param {Array} array An array to be iterated over. | ||
| * @param {Function} fn A function with the signature `(a, index, array) -> b`. |
There was a problem hiding this comment.
JSDuck allows you to explicitly specify callback (fn in this case) signature. You don't have to do it in a custom way.
See https://github.com/senchalabs/jsduck/wiki/%40param#callback-parameters for more info.
core/tools.js
Outdated
| * @param {Array} array An array to be iterated over. | ||
| * @param {Function} fn A function with the signature `(a, index, array) -> b`. | ||
| * @param [thisArg=undefined] The context object for `fn`. | ||
| * @return {*} |
tests/core/tools/array.js
Outdated
|
|
||
| assert.isUndefined( ret, 'Undefined should be returned.' ); | ||
|
|
||
| assert.isTrue( CKEDITOR.tools.objectCompare( arr, results ), 'Each array item should be iterated.' ); |
There was a problem hiding this comment.
There's a dedicated arrayAssert object that contains better asserting methods for what you're trying to do here.
tests/core/tools/array.js
Outdated
|
|
||
| assert.isUndefined( ret, 'Undefined should be returned.' ); |
There was a problem hiding this comment.
Undefined should be returned it is not a good practice to put expected value if you use proper assertion.
In this case you used the proper assertion, assert.isUndefined() which already tells exactly what value is expected. If assertion fails, there is an information that undefined was expected. And this should be the only source of truth.
Duplicating this information in assertion msg you add a risk that someone will change the method, but will leave msg unchanged.
Simply make it value agnostic, but keep the information what has failed, like: "Proper value is returned" or even simpler as we do in many other places "Returned value".
tests/core/tools/array.js
Outdated
| ret = this.array.find( arr, function( item, index ) { | ||
| results.push( item ); | ||
|
|
||
| assert.areSame( arr.indexOf( item ), index, 'Index argument should match item index.' ); |
tests/core/tools/array.js
Outdated
| } | ||
| }, | ||
|
|
||
| 'test array.find': function() { |
There was a problem hiding this comment.
Technically you're doing two test cases in one. Please, split it into two clearly defined test cases.
tests/core/tools/manual/find.html
Outdated
| @@ -0,0 +1,27 @@ | |||
| <style> | |||
There was a problem hiding this comment.
This is purely API addition and should not be tested manually, as it's possible to test it 100% using automatic testing.
engineering-this
force-pushed
the
t/2700
branch
2 times, most recently
from
a3d7245
to
7132201
Compare
…age. Changed some assertions to use dedicated arrayAssert.
What is the purpose of this pull request?
New feature
Does your PR contain necessary tests?
All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.
This PR contains
What changes did you make?
Added new array tool:
CKEDITOR.tools.array.findCloses #2700