[BREAKING] Fix #3602 let setDistinct keep the original order

[dhimansachit]

Tests passing

[josdejong]

Thanks @dhimansachit for working out a solution with tests!

Can you please undo the formatting changes in the file? It's hard to see what actually changed.

[dhimansachit]

Thanks @dhimansachit for working out a solution with tests!

Can you please undo the formatting changes in the file? It's hard to see what actually changed.

Hi @josdejong reverted the formatting changes. Can you please review once now?

[josdejong]

Thanks for the update @dhimansachit ! This looks good 👌

I made one inline remark, can you have a look at that?

[josdejong]

How about using the array find method or some or every? For example:

if (result.every(item => compareNatural(b[i], item) !== 0)) { 
  result.push(b[i])
}

[dhimansachit]

Sure. Updated the logic to use some().

[AnslemHack]

Hi @dhimansachit, thanks for sending this PR I really appreciate the effort you’re putting into improving mathjs. however For readability and maintainability, could you extract the inline function into a small helper with a descriptive name? That way the intent is clearer at a glance. For example:

const containsItem = (arr, value) =>
  arr.some(item => compareNatural(value, item) === 0);

if (!containsItem(result, b[i])) {
  result.push(b[i]);
} 

@josdejong if you don't mind

[josdejong]

Thanks for thinking along Anslem.

I'm OK with both inline or helper function. Do you have any preference @dhimansachit?

[dhimansachit]

I think it's better that it's inline as the function is not used anywhere else. But I'm fine with the other approach as well. Please let me know if you want me to update the logic to contain a function or const for storing the found elements.

[josdejong]

I see that some unit tests are failing now, I guess those are the existing unit tests that now return a differing order.

[dhimansachit]

I see that some unit tests are failing now, I guess those are the existing unit tests that now return a differing order.

Sorry, my bad. Didn't update the previous test. Updated now in the latest commit.