add constructorof for Dict

[rafaqz]

Allows setting Dict fields.

[codecov-commenter]

Codecov Report

Merging #47 (e233fcc) into master (f7e276c) will not change coverage.
The diff coverage is 100.00%.

❗ Current head e233fcc differs from pull request most recent head acfc6f6. Consider uploading reports for the commit acfc6f6 to get more accurate results

@@            Coverage Diff            @@
##            master       #47   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines           71        74    +3     
=========================================
+ Hits            71        74    +3     

Impacted Files	Coverage Δ
src/nonstandard.jl	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7e276c...acfc6f6. Read the comment docs.

[jw3126]

This one is a bit tricky since the fields of Dict are mostly private and I am not familiar with the implementation of Dict. But I think doing something like d3 = setproperties(d1, keys=["x", "y"]) can easily result in a buggy dict. What is you use case?

[aplavin]

I think this PR is useful and wouldn't have consistency issues if defined with some restrictions.

I actually tried recently @modify(f, dct.vals |> Elements()) hoping that it would work out of the box, but it didn't.
Dict.vals seems to be the only property that makes sense to set separately. Others, like keys and slots, are tightly related to each other and changing them can easily bring the dict to an inconsistent state.

So, something like this would make sense:

function setproperties(d::Dict, patch::NamedTuple{(:vals,)})
    K = keytype(d)
    V = eltype(patch.vals)
    Dict{K,V}(d.slots, d.keys, patch.vals, d.ndel, d.count, d.age, d.idxfloor, d.maxprobe)
end

[jw3126]

@aplavin, this is better than exposing all internals, but even the vals field is private I think. So why not overload set(o::Dict, ::typeof(values), new_values) instead?

[aplavin]

values() is not the same as vals. vals is a dense array larger than the actual length(dict). Some of its elements are actual dict values, others are uninitialized. So, potential @set dct.vals = map(f, dct.vals) is the most efficient way to transform dict values, assuming the f function is "garbage in - garbage out" and doesn't throw on uninitialized values.

I'm not really arguing this should be included into ConstructionBase - maybe not. Just found this PR when trying set(dict.val).

[jw3126]

Ah thanks for the explanation that makes sense! I guess then I would still take the function lens approach:

_vals(d::Dict) = d.vals
Accessors.set(d::Dict, ::typeof(_vals), new_vals) = ...

[aplavin]

Regarding Accessors, I've also wondered whether @modify(f, dict |> values |> Elements()) can be implemented as basically dict.vals = map(f, dict.vals) (plus some checks). Doesn't look directly possible, because both values() and Elements() need to be considered at once. The operation cannot be decomposed as "get values(dict)", "map f over them", "set values back".

For this reason, I've earlier added the Values() optic to AccessorsExtra (doesn't use this optimized implementation yet). But ideally this optic would just be Elements() ∘ values, if this composition could somehow be treated as one unit.

[jw3126]

Ah yeah that is a much better approach. I think map(f, is the only sane operation you can perform on the vals field, so it does not make much sense to expose it via a function lens.