fix: insert inserts 1 more item than expected for rational index

[dennistruemper]

Fixes #issue_nr - no issue, just PR

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Does your submission pass tests?
2. Have you lint and formatted your code locally prior to submission?

Changes to Core Features:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?
• Have you successfully ran tests with your changes locally?

Hi @vitoke ,
first thanks for this project I really like the ideas and I did some performance testing: WOW

I tried appending to lists, prepending list and inserting in the middle of a list. To be honest, I might not have been the best Idea to use length/2 to find the index to insert to, but I did not expect the list to get 2 extra items instead of just the one I wanted to insert.

In my opinion it should not happen. Reason was insert uses splice, and splice uses take and take had a problem when rational numbers are used as index. One more time it makes me sad, typescript has no concept of integer.

The first commit is to create tests to show the error. Second commit is the fix, to use Math.floor on input index in take.

Test placement might not be ideal, this project is huge!

Thanks for your work,
Dennis

[vitoke]

Hey @dennistruemper,

Thanks for finding and fixing this. Indeed it is a direct result of a flaw/feature of JavaScript not having integers. I chose early on not to sanitize inputs everywhere (which would lead to even more test cases). But in this case I agree the resulting behavior is certainly unexpected.

I think in some situations where performance is key, I used the trick to do n & n instead of Math.floor(n). But I'm not sure what the exact performance benefits are, and in this case I think Math.floor is fine.

Thanks so much for the compliments and for submitting the PR, really happy that more people are starting to get interested in the project! Cheers!

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (5883b52) 96.42% compared to head (92ccf15) 96.43%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #174   +/-   ##
=======================================
  Coverage   96.42%   96.43%           
=======================================
  Files         298      298           
  Lines       51888    51889    +1     
  Branches     6073     6083   +10     
=======================================
+ Hits        50033    50038    +5     
+ Misses       1844     1840    -4     
  Partials       11       11           

Impacted Files	Coverage Δ
.../list/src/custom/implementation/leaf/leaf-tree.mts	98.31% <100.00%> (+0.48%)	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[dennistruemper]

I think you know, sometimes it is fun to explore a new project structure and hunt down a nasty bug. Was my first contact with yarn monorepo. So I learnt a few things while fixing the bug :-)
It was also easy to recreate, so I thought I might try to find a fix. In a codebase with this kind of tests and code organisation it wasn't hard to find the right spot to fix. (Maybe because it was not in the middle of a performance optimisation ^^ intention of Math.floor vs intention of n & n ;-) )

To be honest, after the timebox of a night I would have opened an issue. But it was enough time :-)

Have a good time!