Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Surprising return from insert_at #24

Closed
jdeisenberg opened this issue Mar 31, 2019 · 5 comments
Closed

Surprising return from insert_at #24

jdeisenberg opened this issue Mar 31, 2019 · 5 comments

Comments

@jdeisenberg
Copy link
Contributor

jdeisenberg commented Mar 31, 2019

This surprised me:

insert_at ~index:(-1) ~value:999 [100;101;102;103] = [999]
insert_at ~index:5 ~value:999 [100;101;102;103] = [999]

Looking at the code, this happens because take and drop return the empty list when given values that are out of bounds.

However, I would expect the function to do one of these:

  • Return a list the same as the original when given values that are out of bounds, or
  • Insert the value at the beginning for negative values, and at the end for values greater than the list length
@pbiggar
Copy link
Member

pbiggar commented Apr 1, 2019

Good point. Let's:

  • treat -1 as wrapping, so it inserts in the last position (similar to python)
  • if the index is greater than the bounds, put it at the end.

@jdeisenberg
Copy link
Contributor Author

Treating -1 as wrapping makes things far more difficult, because people would then expect -2, -3... -len(xs) to work in the same way. I think the “do nothing if out of bounds“ is safest and easiest to implement.

@pbiggar
Copy link
Member

pbiggar commented Apr 1, 2019

Good point, let's do that for now then.

@Dean177
Copy link
Collaborator

Dean177 commented Jun 18, 2020

The behaviour of this has now changed.

There is a test case demonstrating that when the index is greater than the length of the list then the value is appended.

The documentation examples still refer to the old behaviour and there is no test case for a negative index.

@pbiggar
Copy link
Member

pbiggar commented Aug 1, 2021

Handled this in #224.

It looks like the wrapping behaviour was supported in the rescript version, so I made native match it, and added tests.

Happy to have done it the other way, this was just easier.

@pbiggar pbiggar closed this as completed Aug 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants