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

Insert at bugfix #4

Closed
wants to merge 2 commits into from
Closed

Conversation

Noobish1
Copy link

@Noobish1 Noobish1 commented Jun 27, 2018

Hi there, there seems to be an off by one error when inserting at an index when using an Int index. It subtracts twice from the index which leads to it being incorrect. I've removed the subtraction in the insert function which takes a non-int Index as it seems to be correct to expect that to already be off by 1 as you have to explicitly state either head or tail.

I've added a few new tests to go along with it, especially around inserting in the middle of a collection.

@stephencelis
Copy link
Member

Thanks for taking the time to make this PR! @mbrandonw and I were recently discussing whether or not this Int-based convenience API is worth the caveats, and we're trying to think through collections that may have Int indices but not be 0-based. We're going to ruminate on it awhile longer, but feel free to let us know what you think!

@Noobish1
Copy link
Author

No problem, I have been using this NonEmptyArray implementation and wanted to see how this one compared and the thing I found most awkward/annoying was the enum Index being used, it feels like its leaking implementation details when the consumer of the API shouldn't have to worry about head and tail really. It gets weird when you want to subscript with a range e.g. array[.head...(.tail(4)].

@stephencelis stephencelis mentioned this pull request Feb 25, 2020
@Noobish1 Noobish1 closed this Oct 19, 2020
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

Successfully merging this pull request may close these issues.

2 participants