-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add mergewith[!](combine, dicts...) #34296
Conversation
I would prefer it if the function form were called something like |
That would be great. Can we also have the curried version |
@andyferris @oxinabox Maybe you are interested in this discussion as this increases dictionary API. |
I added |
Is the probem with Is the odd thing about this method rather the "implict reduction" over varargs dictionaries? The (long) discussion at #32860 seems to at times suggest such implicit folding/reducing might be superceded by an explicit one - leaving here just |
I agree that it sounds problematic to introduce AFAICT there's no ambiguity between |
Yes, for example, In principle, we can do this without introducing right(_, x) = x
merge!(dest::MyType, other, rest...) = _merge_impl(right, dest, other, others...)
merge!(combine, dest::MyType, other, rest...) = _merge_impl(combine, dest, other, others...)
# Disambiguation:
merge!(dest::MyType, other::MyType, rest...) = _merge_impl(right, dest, other, others...) I think it's OK. But it's ugly; especially the disambiguation
I think we only need this for "monoid-like" functions; i.e., the ones with signatures " |
I have to admit, the callable amibuguity is a bit annoying. You can't add a type to the We do have a pattern for dealing with these things - traits. Speculatively, we could define in export iscallable
iscallable(::Any) = false
iscallable(::Function) = true
iscallable(::Type) = true and query |
Actually, doesn't |
I agree defining trait is a possible approach, but why bring such an "advanced" solution when you have a dumb solution? It also makes it harder to know what I think it's a nice property that you can tell what
I think it does. But I think the list of |
PyCall are indeed the most problematic example, but it's use with |
I don't understand why PyCall example is hypothetical. Personally, I'd use it if it were usable. Also, that's exactly the reason why the type restriction of In Julia, everything is potentially callable. I think it'd be nice if |
@tkf - something in particular must have motivated you to create this PR - it is often helpful to understand these cases, so I feel the question is fair. Was there something you were trying to do that didn't work for you?
Agreed. |
OK. That's a fair point. I just didn't want to bring it up as it's not a clean example. It came up when I tried something like julia> using Transducers: Map, reducingfunction
julia> rf = reducingfunction(Map(x -> 2x), +)
Reduction(
Map(Main.λ❓),
BottomRF(
+))
julia> merge(rf, Dict(:a => 1), Dict(:a => 1))
ERROR: MethodError: no method matching merge(::Transducers.Reduction{Map{var"#3#4"},Transducers.BottomRF{typeof(+)}}, ::Dict{Symbol,Int64}, ::Dict{Symbol,Int64})
Closest candidates are:
merge(::OrderedCollections.OrderedDict, ::AbstractDict...) at /home/takafumi/.julia/packages/OrderedCollections/E21Rb/src/ordered_dict.jl:434
merge(::DataStructures.SortedDict{K,D,Ord}, ::AbstractDict{K,D}...) where {K, D, Ord<:Base.Order.Ordering} at /home/takafumi/.julia/packages/DataStructures/iymwN/src/sorted_dict.jl:598
merge(::AbstractDict, ::AbstractDict...) at abstractdict.jl:284
...
Stacktrace:
[1] top-level scope at REPL[9]:1
julia> merge((args...) -> rf(args...), Dict(:a => 1), Dict(:a => 1))
Dict{Symbol,Int64} with 1 entry:
:a => 3 But I thought that it was not a good example as it is "fixable" and, being the library author, arguably I should. I can make whatever returned by However, I don't think tweaking the type hierarchy is the right approach. |
With multiple dispatch, just as the meaning of a function is important to get consistent, it's also important to get the meanings of the argument slots consistent when possible. It's not ideal for a function to treat an argument in a fundamentally different way based on its type: e.g. "if this is a Function, then call it on pairs of values, otherwise assume it's a container providing the values to use". Given the meaning of |
Co-Authored-By: Jameson Nash <vtjnash@gmail.com>
Co-Authored-By: Jeff Bezanson <jeff.bezanson@gmail.com>
Can we remove the restriction
::Function
inmerge[!](combine::Function, d::AbstractDict, others::AbstractDict...)
? Was this there to disambiguate the case where the first argument isAbstractDict
but also is callable? Or was it there to allowmerge[!](d::MyDict, ::Any)
?