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

WIP: Implement insert! and deleteat! for OffsetVectors #137

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

garrison
Copy link
Contributor

@garrison garrison commented Sep 3, 2020

I consider this to be incomplete. I have not, for instance, thought carefully about whether I am referring to the axes in the most appropriate way, but hey, all tests pass! Also, ideally the offset indices will be used in the exception message when the index is out of range; right now, the indices of the parent array are instead provided, and this can be confusing.

@garrison
Copy link
Contributor Author

garrison commented Sep 3, 2020

Just found #55 (comment), which did not turn up in my earlier repository search. I still believe implementing insert! and deleteat! is valuable, but obviously it deserves some discussion first. I'll open an issue shortly to discuss the related question of whether to implement pushfirst! and popfirst!.

@garrison garrison marked this pull request as draft September 4, 2020 01:13
@timholy
Copy link
Member

timholy commented Sep 9, 2020

As you can tell from your readings, I'm mixed on this. If I had to take a stance, it would be "against by a hair." Here's why:

  • pushfirst! really seems like it should grow in the "opposite direction", meaning firstindex(a) gets decremented rather than lastindex(a) being incremented
  • insert!(a, firstindex(a), items) should behave the same as pushfirst! if items is of length 1.

@MasonProtter
Copy link
Contributor

@timholy What about deleteat!? I think that's a useful function and it's required for things like filter! and would have been useful test cases for two PRs I recently wrote: JuliaLang/julia#39528 and Vexatos/CircularArrays.jl#12

@timholy
Copy link
Member

timholy commented Feb 7, 2021

We can have this if there's enough enthusiasm. I just think that the consensus has never been clear. Lots of people initially think "why isn't that implemented? I'll add it!" and then once the concerns are raised most eem to agree that it's not obvious we want it. If someone makes a sufficiently clear case and is willing to document the behavior carefully, we can do things like this. But the principle of "do no harm" holds me back from adding this until someone really pushes a strong argument.

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.

3 participants