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

Return the first argument in put! #34202

Closed
wants to merge 2 commits into from
Closed

Conversation

tkf
Copy link
Member

@tkf tkf commented Dec 26, 2019

push! usually returns the first argument ("container")

julia> push!([], 1)
1-element Array{Any,1}:
 1

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

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

but not with channels

julia> push!(Channel(Inf), 1)
1

This PR fixes this inconsistency by tweaking put! such that put!(c, _) === c. This automatically fixes push! as push! is just an alias of put!:

push!(c::Channel, v) = put!(c, v)

This also makes put!(::Channel, _) consistent with put!(::RemoteChannel, _)

"""
put!(rr::RemoteChannel, args...)
Store a set of values to the [`RemoteChannel`](@ref).
If the channel is full, blocks until space is available.
Return the first argument.
"""
put!(rr::RemoteChannel, args...) = (call_on_owner(put_ref, rr, myid(), args...); rr)

@KristofferC KristofferC added kind:minor change Marginal behavior change acceptable for a minor release needs pkgeval Tests for all registered packages should be run with this change labels Dec 26, 2019
@StefanKarpinski
Copy link
Sponsor Member

Perhaps that should be the distinction between push and put for channels?

@tkf
Copy link
Member Author

tkf commented Dec 27, 2019

Sounds reasonable to me. But in this case, ideally, I think put!(::RemoteChannel, v) should then be re-defined to return v and the vararg form put!(::RemoteChannel, args...) should be deprecated (or maybe return args[end]?) for consistency. This is technically more breaking as the docstring for put!(::RemoteChannel, args...) explicitly says "Return the first argument." Perhaps this has to be postponed until 2.0?

Maybe the option with least pain is to just define

push!(c::AbstractChannel, v) = (put!(c, v); c)

and leave put! methods as-is. This would let people use Channel and RemoteChannel with a consistent API.

@TotalVerb
Copy link
Contributor

put! for a Future also returns the first argument, which is very useful for e.g. creating an immediately resolved future put!(Future(), x). So for consistency, I'd also prefer put! to return the channel when the first argument is a channel. It is not too bad to do v = value; put!(c, v); v in the cases where the value is actually needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:minor change Marginal behavior change acceptable for a minor release needs pkgeval Tests for all registered packages should be run with this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants