-
-
Notifications
You must be signed in to change notification settings - Fork 564
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
Simplify types of array/slice and array/base/slice via use of generic type parameter #1318
Conversation
refactor: simplify via use of generic type parameter
*/ | ||
declare function slice<T = unknown>( x: Collection<T>, start: number, end: number ): Array<T>; | ||
declare function slice<T extends Collection | ComplexTypedArray>( x: T, start: number, end: number ): T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we lost some info here. Namely, if T
is a collection without a slice
method, we always return a "generic" array having elements of the same type. The prior definition was a fall back definition for "unknown" collections and "generic" arrays. We can probably do better by defining an interface which extends Collection
and has a slice
method conforming to a particular signature. In that case, T
in and T
out. Otherwise, for a collection without a slice
method, we return a "generic" array having elements of type U
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kgryte I reverted back to the prior definitions for a generic collection. We also have to do this as my proposed type broke down for tuples (say [1,2,3] as const
), which would incorrectly be set as the return type irrespective of what was sliced off.
But we do have another issue that was present before, namely that the definition doesn't work for strings, as it incorrectly asserts that the return value will be an array of strings in this case, though it is just a string. Should we handle this, even though the package is in the array
namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind, that the function does not apply to strings is fine with me and part of the original reason why I typed it as Collection
, rather than ArrayLike
. For users wanting to slice strings, they'd be better off using a dedicated @stdlib/string/slice
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kgryte Makes sense, although I believe strings
do fall under our Collection
s as well, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, no.
See
* A collection, which is defined as either an array, typed array, or an array-like object (excluding strings and functions). |
typeof value === 'object' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strings do fall under the TypeScript definition (
type Collection<T = any> = Array<T> | TypedArray | ArrayLike<T>; // eslint-disable-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things may have changed, but IIRC it may have been influenced by TypeScript 2.1 capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Planeshifter Thanks for the POC. Overall, this will be an improvement. I think we can improve the declarations a bit more to account for implementation details. Once resolved, this should be ready for merge.
7cf52b9
to
66e55bc
Compare
66e55bc
to
7ab004b
Compare
lib/node_modules/@stdlib/array/base/slice/docs/types/index.d.ts
Outdated
Show resolved
Hide resolved
* var out = slice( x, 0, 3 ); | ||
* // returns [ 1, 2, 3 ] | ||
* var out = slice( x, 0, 2 ); | ||
* // returns [ 1, 2 ] | ||
*/ | ||
declare function slice<T = unknown>( x: Collection<T>, start: number, end: number ): Array<T>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to use the interface
approach I mentioned previously, or should we be content with typing like this? Atm, this will return incorrect results for specialized constructors having a slice
method which are not captured by TypedArray | ComplexTypedArray
. Granted, the previous declarations also did not capture this case, but we could go ahead and correct here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kgryte We should merge this PR without the proposed change as I am not getting it to work properly with tuple types. Have some work-in-progress stuff we can discuss but this is quite complicated and would also require some opaque typing, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. We should create a TODO to follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Planeshifter Thanks for the updates. A couple of the examples are incorrect, and I have a question about how we can retain type specificity when an input array has a slice
method.
lib/node_modules/@stdlib/array/base/slice/docs/types/index.d.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Athan <kgryte@gmail.com>
lib/node_modules/@stdlib/array/base/slice/docs/types/index.d.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Athan <kgryte@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks, @Planeshifter!
Description
This pull request:
array/slice
andarray/base/slice
through the use of generic type parameters.Related Issues
No.
Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers