feat(List): add move function

[MadDeveloper]

As discussed here: #2692

[CrossEye]

Thank you for the PR. There are several suggestions here, and a question for the @ramda/core team: As well as the obvious question of whether you think this function is worth including, I also would like to know if you think it should apply to any indexable type.

[CrossEye]

Although I'm the one who suggested that this be done on the native methods, I'm having some second thoughts. Would this do well working on all indexable types? Strings would be an obvious fit, but should it also work on NodeList, on ArgumentList, and so on?

What do you think, @ramda/core?

[CrossEye]

But is this the correct behavior? Why do we distinguish how it behaves with the source index and the destination one?

[MadDeveloper]

Because of the flexibility of move() and more precisely splice and slice, we are able to make a list rotation, but it has some unexpected behaviour, like this one you pointed out. How do you think prevent this behaviour without breaking the rotation one? Should we prevent only the positive outbounds? Because the negative outbounds can be a desired behaviour.

[CrossEye]

I'm not quite sure. Perhaps we want to allow negative outbounds only down to the first element. I really don't like this asymmetry:

move(-50, 2, ['a', 'b', 'c', 'd', 'e', 'f']) //=> ['b', 'c', 'a', 'd', 'e', 'f']
move( 50, 2, ['a', 'b', 'c', 'd', 'e', 'f']) //=> ['a', 'b', 'c', 'd', 'e', 'f']

[MadDeveloper]

Maybe these ones are enough:

move(0, -1, ['a', 'b', 'c']) /// ['b', 'c', 'a']
move(-1, 0, ['a', 'b', 'c']) /// ['c', 'a', 'b']

[CrossEye]

That also seems odd to me. If we're going to allow negative indices, these seem reasonable too:

move(-2, 1, ['a', 'b', 'c', 'd', 'e', 'f']) //=> ['a', 'e', 'b', 'c', 'd', 'f']
move(1, -2, ['a', 'b', 'c', 'd', 'e', 'f']) //=> ['a', 'c', 'd', 'e', 'b', 'f']

Generally, if -1 gets special treatment it's as a not-found signal, as in Array.prototype.findIndex

[MadDeveloper]

It seems better. It looks like a first version I wrote, but that I've lightened in order to remove check verifications as I didn't know if Ramda desired them: Here is the tests that were in the move function:

// all exclusive tests
if (typeof from !== 'number' 
  || typeof to !== 'number'
  || from > length - 1
  || from < 0
  || to > length - 1
  || to < 0
  || from === to
  || length === 0) {
  return result
}

Negative indices were totally excluded.
Rewriting them with positiveFrom and positiveTo can do the job perfectly and make more sense.

[CrossEye]

Do you want to do that update? I'd be inclined to accept this with that change, although I would still like some feedback from @ramda/core .

[MadDeveloper]

Updated. I didn't add the realIndex function in the internal directory, as I don't know if it should be added in a separate PR. Let me know if you want to see it appears in this PR.

[CrossEye]

Well, I'm afraid that this code combines two of what I think of contradictory suggestions of mine.

I was suggesting something like this except with

  var positiveFrom = from < 0 ? list.length + from : from;
  var positiveTo = to < 0 ? list.length + to : to;

so that

move(-2,  1, ['a', 'b', 'c', 'd', 'e', 'f']) //=> ['a', 'e', 'b', 'c', 'd', 'f']
move( 1, -2, ['a', 'b', 'c', 'd', 'e', 'f']) //=> ['a', 'c', 'd', 'e', 'b', 'f']
move(-50, 2, ['a', 'b', 'c', 'd', 'e', 'f']) //=> ['a', 'b', 'd', 'd', 'e', 'f']
move( 50, 2, ['a', 'b', 'c', 'd', 'e', 'f']) //=> ['a', 'b', 'c', 'd', 'e', 'f']

The idea is that a negative index only does something up to the length of the list. After that, it becomes a no-op. I think that stopping at the end of the array is far too surprising.

[MadDeveloper]

Ok, I think there was a misunderstanding... I fixed it and I hope everything is ok now.

[CrossEye]

Merged. Thank you for your persistence.