feat: add slice method to array/fixed-endian-factory
#3188
Conversation
|
Hello! Thank you for your contribution to stdlib. We noticed that the contributing guidelines acknowledgment is missing from your pull request. Here's what you need to do:
This acknowledgment confirms that you've read the guidelines, which include:
We can't review or accept contributions without this acknowledgment. Thank you for your understanding and cooperation. We look forward to reviewing your contribution! |
|
kindly review this PR!! |
Coverage Report
The above coverage report was generated for the changes in this PR. |
|
/stdlib merge |
kgryte
changed the title
slice method to array/fixed-endian-factory
…method_to_array/fixed-endian-factory
|
/stdlib lint-autofix |
| var str = arr.toString(); | ||
| // returns '1,2,3' | ||
| ``` | ||
| #### TypedArrayFE.prototype.slice() |
There was a problem hiding this comment.
See https://github.com/stdlib-js/stdlib/tree/4b1d77d2bd001d5970ce93230765a579fb41349d/lib/node_modules/%40stdlib/array/bool#booleanarrayprototypeslice-start-end- for what is excepted in terms of documentation. Additionally, this documentation should be inserted such that methods are listed in alphabetical order.
There was a problem hiding this comment.
You are missing a separate benchmark file which evaluates performance as a function of array length. E.g., https://github.com/stdlib-js/stdlib/blob/4b1d77d2bd001d5970ce93230765a579fb41349d/lib/node_modules/%40stdlib/array/bool/benchmark/benchmark.slice.length.js
| * @returns {TypedArray} string representation | ||
| */ | ||
|
|
||
| setReadOnly( TypedArray.prototype, 'slice', function slice() { |
There was a problem hiding this comment.
Insert this method so that methods are listed in alphabetical order.
| }); | ||
|
|
||
| /** | ||
| * return sliced TypedArray. |
There was a problem hiding this comment.
Your code should look very similar to
. Currently, it does not.
| * @memberof TypedArray.prototype | ||
| * @type {Function} | ||
| * @throws {TypeError} `this` must be a typed array instance | ||
| * @returns {TypedArray} string representation |
| /** | ||
| * return sliced TypedArray. | ||
| * | ||
| * @private |
| if (value2 === null || nargs === 1) { | ||
| value2 = this._length; | ||
| } | ||
| if ( nargs < 0 || nargs > 2 || !isNonNegativeInteger( value1 ) || !isNonNegativeInteger( value2 ) ) { |
There was a problem hiding this comment.
Should accept negative integers, not just nonnegative integers.
| if ( !isTypedArray( this ) ) { | ||
| throw new TypeError(format('invalid invocation. `this` is not %s %s.', CHAR2ARTICLE[ dtype[0]], CTOR_NAME)); | ||
| } | ||
| temp = []; |
There was a problem hiding this comment.
You don't need this temporary array. As in
, you can create a new instance and then copy elements directly to the target array DataView buffer.
|
|
||
| // TESTS // | ||
|
|
||
| tape('main export is a function', function test( t ) { |
There was a problem hiding this comment.
Please follow stdlib spacing conventions consistently throughout this file.
There was a problem hiding this comment.
Your tests should essentially follow the same structure as https://github.com/stdlib-js/stdlib/blob/4b1d77d2bd001d5970ce93230765a579fb41349d/lib/node_modules/%40stdlib/array/bool/test/test.slice.js. For every set of tests in that package, I would expect to see similar tests in this package.
kgryte
added
the
Needs Changes
|
CI failures will need to be resolved before this PR can receive further review. Cheers! |
|
@kgryte please review PR#3269 i have made the necessary changes told by you above |
DhruvArvindSingh
deleted the
#3154_add_slice_method_to_array/fixed-endian-factory
branch
3 participants
Resolves #3154 .
Description
This pull request:
Related Issues
This pull request:
slicemethod toarray/fixed-endian-factory#3154Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers