Implement mergewith() for OrderedDict and LittleDict #119
Conversation
src/little_dict.jl
Outdated
| return dc | ||
| end | ||
|
|
||
| mergewith(combine::Function, d::LittleDict, others::AbstractDict...) = merge(combine, d, others...) |
There was a problem hiding this comment.
Using Function is very restrictive, it excludes a bunch of useful applications, e.g. where combine is a type (so expressing coercion). However just making this Any can lead to ambiguities in the merge method. Avoiding that is AFAIK precisely the reason why mergewith was introduced in the first place.
So instead of implementing mergewith in terms of merge it is better to do it the other way around.
Thus I suggest that instead of adding this here, you instead rename the preceding merge method starting at line 169 to mergewith and drop the ::Function from its first argument.
Then here you can do
| mergewith(combine::Function, d::LittleDict, others::AbstractDict...) = merge(combine, d, others...) | |
| merge(combine::Function, d::LittleDict, others::AbstractDict...) = mergewith(combine, d, others...) |
or even better (matching what the mergewith docstring says):
| mergewith(combine::Function, d::LittleDict, others::AbstractDict...) = merge(combine, d, others...) | |
| merge(combine::Union{Function,Type}, d::LittleDict, others::AbstractDict...) = mergewith(combine, d, others...) |
src/ordered_dict.jl
Outdated
| merge!(combine, OrderedDict{K,V}(), d, others...) | ||
| end | ||
|
|
||
| mergewith(combine::Function, d::OrderedDict, others::AbstractDict...) = merge(combine, d, others...) |
There was a problem hiding this comment.
As above I suggest to generalize this so that combine can be anything. This should work:
| mergewith(combine::Function, d::OrderedDict, others::AbstractDict...) = merge(combine, d, others...) | |
| function mergewith(combine, d::OrderedDict, others::AbstractDict...) | |
| K,V = _merge_kvtypes(d, others...) | |
| mergewith!(combine, OrderedDict{K,V}(), d, others...) | |
| end |
Optionally the preceding merge method could then be turned into a one-liner calling mergewith.
|
Just to be clear: I am not a maintainer of this project, feel free to ignore me. Also, regardless of anything else: thank you for making the effort and contributing to this package, much appreciated. (I should have started with that, sorry) |
|
Thanks @fingolfin, makes sense! I had no idea why |
As suggested by @fingolfin, implementing `merge()` with a `combine::Function` argument in terms of `mergewith()` allows the signature of `mergewith()` to not restrict `combine` to be a `Function` specifically, which can be useful in some situations.
fingolfin
left a comment
fingolfin
left a comment
There was a problem hiding this comment.
Looks good to me, thank you :-)
2 participants
Fixes #77.