Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Simplify types of array/slice and array/base/slice via use of generic type parameter #1318
Changes from 1 commit
3c06d16
7ab004b
e572eb0
0067d03
e1d0be8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 aslice
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 extendsCollection
and has aslice
method conforming to a particular signature. In that case,T
in andT
out. Otherwise, for a collection without aslice
method, we return a "generic" array having elements of typeU
.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 thanArrayLike
. 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 ourCollection
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
stdlib/tools/docs/jsdoc/typedefs/objects.js
Line 54 in 4315e33
stdlib/lib/node_modules/@stdlib/assert/is-collection/lib/main.js
Line 45 in 4315e33
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 (
stdlib/lib/node_modules/@stdlib/types/index.d.ts
Line 595 in 4315e33
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.