added array-upsert package#499
Conversation
|
Thanks this is really nice! |
|
I prefer the more straightforward simplistic logic, eg; Does the index exist in the array?
I think the extra logic behind "no" with "is the index negative" goes outside the scope of this function, and its very easy for someone to check that on their own. I agree that its rare someone would pass a negative index to the function in order to prepend, but ultimately, I think keeping the function straightforward is more worth it. Someone reading code who saw this function could reasonably assume how it functions, and they probably would not think it would prepend if the array is negative. I always prefer to write functions that are narrow in scope, but that's just my opinion |
Same. Ok sounds good. |
angus-c
left a comment
angus-c
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing this. A few smallish things — biggest is I don't think we should limit type of upserted element to be the same as the type in the array.
| @@ -0,0 +1,21 @@ | |||
| The MIT License (MIT) | |||
|
|
|||
| Copyright (c) 2016 angus croll | |||
| import upsert from 'just-upsert'; | ||
|
|
||
| upsert([1,2,3,4],-1,2) // [1,2,-1,4] | ||
| upsert(['a','b','c'],'d',6) // ['a','b','c','d'] |
There was a problem hiding this comment.
Add example with negative third arg too, just to resolve ambiguity
| upsert([1, 2, 3], 5, 'a'); | ||
|
|
||
| // @ts-expect-error | ||
| upsert(['a', 'b', 'c'], 5, 2); |
There was a problem hiding this comment.
I think this should be valid (see comment in d.ts file)
| @@ -0,0 +1,3 @@ | |||
| declare function upsert<T>(arr: T[], newValue: T, targetIndex: number): T[]; | |||
There was a problem hiding this comment.
I don't think we should limit new value to type of existing elements.
So upsert([1,2,3], 'a', 2) should be valid
| @@ -0,0 +1,223 @@ | |||
| const upsert = require('../../packages/array-upsert'); | |||
|
@angus-c sorry for the huge delay, got distracted by stuff and forgot about this entirely, You can actually add an item of a different type to an array by manually specifying the generics, which I accounted for in the upsert<number | string>(['a', 'b', 'c'], 5, 2);I've also updated the package to match the newer structure |
3 participants
Closes #481