emmanueltouzery / prelude-ts

Functional programming, immutable collections and FP constructs for typescript and javascript
ISC License
377 stars 21 forks source link

Provide operations insert and insertAll from funkia list #29

Closed Hraun closed 5 years ago

Hraun commented 5 years ago

I propose implementing missing operations from funkia list, so that the prelude-ts vector takes full advantage of its strengths. In this commit I've delegated insert and insertAll to funkia list. If you concur with this approach, there are a few further operations which we could add to vector, such as slice.

emmanueltouzery commented 5 years ago

Thank you for the pull request! In general, I don't want to add too many operations to prelude-ts just for the sake of it, but certainly I'm very open to adding them if it's "worth it" (and obviously that's hard to define).

Maybe I would start with slice which you mention as well, because that seems the more clear-cut case for me. Obviously I thought about adding slice previously, but I think we're better off by skipping it and having users rather use combinations .take() and .drop(). With slice(), or with 2+ parameters functions like that, I feel it's never 100% obvious for someone who doesn't know the API very well, whether it's start position and length.. or start position and end position. If you have a chain with take and drop on the other hand, it's crystal clear. I think that clarity is worth a lot. In terms of performance, I really doubt it would ever be measurable that two calls would be worse than one, except maybe in the most artificial micro-benchmarks.

Now of course if you think adding slice is really the right thing to do, I'm listening, but that's my starting opinion let's say. Also if we added slice we'd probably add it to Sequence so that also LinkedList and Stream would get it. That's not the case for insert and insertAll because you wouldn't wand to work with indexes on LinkedList and Stream.

Regarding insert and insertAll, I feel it's less clear-cut. I mean one can still do take() then add() then drop() then add() but that's clearly not "simpler". Working with indexes is not really the functional style though, and prelude-ts wants to be used in a functional way. Honestly I've not needed something like these functions for years, but I understand they can come in handy.

What do you think about adding the feature, but we could have just one method, which we could name maybe insertAt to make it clearer maybe what's the purpose, and would be the one taking a list as the second parameter? We could try to make a benchmark and if it shows that there is a measurable difference between insert and insertAll then we could add both versions. If yes, we can talk about the naming, I'm not sure then :-)

Also, can you add a code sample in the apidoc? The tests actually extract the code samples from the apidoc and run them, you can see in some other APIs.

You must indent the code samples by 4 characters and give the result on the next line after =>. The rules for multi-line samples are a little tricky though. If you don't get it right at first, just drop it and I'll make it work.

Example of a function having such apidoc checked by tests: http://emmanueltouzery.github.io/prelude.ts/latest/apidoc/classes/vector.html#sliding

emmanueltouzery commented 5 years ago

I would prefer not to have an "overloaded" version taking T|Iterable<T> btw (then it's "simpler" to have two names). I don't know, maybe we can really have two methods, like insertAt and insertAtIterable or something like that. I'm interested in your opinion on that topic.

emmanueltouzery commented 5 years ago

closing due to inactivity, but i'm interested in contributions for sure! I just don't want to add features "because we can". Too many methods introduce confusion. Each feature should have a clear use-case and be as simple as possible. You're always welcome to make suggestions and PRs! Thank you for your contribution.