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

Semantic clash with Base.insert! #16

Open
c42f opened this issue Apr 24, 2020 · 7 comments
Open

Semantic clash with Base.insert! #16

c42f opened this issue Apr 24, 2020 · 7 comments

Comments

@c42f
Copy link
Collaborator

c42f commented Apr 24, 2020

insert!(dict, k, val) with a dictionary means something very different from insert! with a Vector. The Vector API from Base is more or less sequence- or iterable-like: insert something at a given index of the sequence, and move the remaining values to make room, preserving iteration order.

I wondered whether this is an oversight, or whether there's a plan to deal with this in some way?

@tkf
Copy link

tkf commented May 28, 2020

How about push!(dict, k => v)? It works with AbstractDict and AbstractSet:

julia> push!(Dict(), :a => 1)
Dict{Any,Any} with 1 entry:
  :a => 1

julia> push!(IdDict(), :a => 1)
IdDict{Any,Any} with 1 entry:
  :a => 1

julia> push!(Set(), :a)
Set{Any} with 1 element:
  :a

julia> push!(Base.IdSet(), :a)
Base.IdSet{Any} with 1 element:
  :a

@c42f
Copy link
Collaborator Author

c42f commented May 29, 2020

push! seems approximately right. Does it matter that AbstractDictionary is not really a container of pairs though? This may lead to oddities, such as after push!(d, e), collect(d) will not contain e?

@tkf
Copy link

tkf commented May 29, 2020

This may lead to oddities, such as after push!(d, e), collect(d) will not contain e?

Ah, very good point. Actually, insert! looks kind of appropriate now (at least within the functions defined in Base) for me.

insert something at a given index of the sequence, and move the remaining values to make room, preserving iteration order.

I think insert!(::AbstractDictionary, i, v) does the first point right. I don't think the last point is really necessary as iteration order is (usually) arbitrary for dictionaries. The second point ("move the remaining values") is unfortunate but maybe it can be compared to the case where you don't need to "make room"?

julia> insert!([:a, :b, :c], 4, :d)
4-element Array{Symbol,1}:
 :a
 :b
 :c
 :d

@tkf
Copy link

tkf commented Jun 1, 2020

Reading JuliaLang/julia#35513 and JuliaLang/julia#36070, I wonder if vectors should implement insertat! and let AbstractDictionary and AbstractDict define insert!. (With the compatibility fallback for insert!(vector, ...), of course.)

@c42f
Copy link
Collaborator Author

c42f commented Jun 1, 2020

Good point. The semantic mismatch between Base insert! and delete! seems very unfortunate.

@andyferris
Copy link
Owner

My thinking here is that Vector should use insertat!

@c42f
Copy link
Collaborator Author

c42f commented Jun 12, 2020

Agreed. Inserting-into-a-sequence and implicitly shifting the keys (indices) of the rest of the sequence seems like a fairly specialized operation. Not something that many kinds of collection will want to implement, so having a specific rather than generic name for it would be an improvement.

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