`index` for `rm` appears to show _last_ index removed, not the first

[justinanastos]

I found this while trying to get the previous values that were set to show an actually +/- diff.

const originalArray = ['a', 'b', 'c', 'd', 'e'];
const newArray = ['a', 'e'];
const diff = odiff(originalArray, newArray);

Will return:

[
  {
    "type": "rm",
    "path": [],
    "index": 3,
    "num": 3
  }
]

num is correct, 3 elements were removed. index is the last index that was removed; d in this case. I would expect index to be 1, the first element removed.

If index was 1, then it's trivial to get the original elements removed:

const elementsRemoved = originalArray.slice(diff.index, diff.index + diff.num);

[fresheneesz]

Hmm, definitely not what I intended. Do you think you could submit a pull request fix?

[kevinludwig]

Would it be sufficient to simply change the rm method (here: https://github.com/Tixit/odiff/blob/master/odiff.js#L120) to set index: index - count + 1?

Note that the reason current tests do not catch this is because all the tests only try to add/remove one item (in which case the first/last index are identical). You need a test that removes 2 or more contiguous elements for this to happen.

[kevinludwig]

https://github.com/Tixit/odiff/pull/4

Not sure how you want to version bump this @fresheneesz since it theoretically could be viewed as breaking for users who relied on the current behavior.

[fresheneesz]

Thanks for the pull request! Yeah, good point about the version. I'll bump it a whole version.

[fresheneesz]

Published under v1.0.0

[cajurabi]

@fresheneesz is this closed? I routinely review open issues list before deciding to use a package, and I believe your open issues, should probably be closed. Am I correct? Or are the open issues still issues for the package?

[fresheneesz]

@cajurabi Ah yes, this should be closed