Closed fatteneder closed 2 years ago
I think you don't need a _from_dict_errorhandled
but just rethrow an error at
https://github.com/Roger-luo/Configurations.jl/blob/master/src/from_dict.jl#L109
will do this? I'd like to keep from_dict
used in the generated functions
Merging #69 (41eb344) into master (933fd46) will increase coverage by
0.06%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #69 +/- ##
==========================================
+ Coverage 97.27% 97.34% +0.06%
==========================================
Files 12 12
Lines 588 603 +15
==========================================
+ Hits 572 587 +15
Misses 16 16
Impacted Files | Coverage Δ | |
---|---|---|
src/Configurations.jl | 100.00% <ø> (ø) |
|
src/errors.jl | 100.00% <100.00%> (ø) |
|
src/from_dict.jl | 94.14% <100.00%> (+0.11%) |
:arrow_up: |
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 933fd46...41eb344. Read the comment docs.
Initially I implemented it as from_dict
, but then noticed that this is just the workaround mentioned in #68, e.g. you specify a default behavior.
This has the problem that whenever you override from_dict
for a custom option type, you can't catch conversion errors thrown in there.
I tried to mimic this with this test here: https://github.com/Roger-luo/Configurations.jl/pull/69/files#diff-2b7924e7265e1f90cd05070d90cd82aa631d0e3417d99c2d39ce8d34cc4712e9R71
Also from_dict
with four arguments has two implementations, e.g.
AbstractVectors
https://github.com/Roger-luo/Configurations.jl/blob/d5d0fbbc53f0703f4b9486fb367d687402cc9bf6/src/from_dict.jl#L133-L135Hence, I wrapped from_dict
into _from_dict_errorhandled
that does the exception handling separately.
try-catch
blocks, because the _from_dict_errorhandled
will be called on each level of the nested dict.FieldTypeConversionError
will only trigger if a Base.convert
throws inside from_dict
. So if someone implements a from_dict
for a custom type (and field) that does not use Base.convert
, then the caller will be left with an unspecific error as before. If we don't care about errors caused by a custom from_dict
implementations, then the _from_dict_errorhandled
is indeed redundant.
Please let me know what you prefer.
Yeah I think we don’t need to care about the custom conversion-it’s gonna be strange if the error is different from what’s implemented by one’s own code
Ok, I see. I changed it back now.
Just noticed another caveat:
julia> @option struct A
s::String
end
julia> @option struct B
a1::A
a2::A
end
julia> d = Dict("a1"=>Dict("s"=>:str), "a2"=>Dict("s"=>"str")))
Dict{String, Dict{String, Symbol}} with 1 entry:
"a1" => Dict("s"=>:str)
"a2" => Dict("s"=>"str")
julia> from_dict(B, d)
ERROR: FieldTypeConversionError: conversion from Symbol to type String for field s in type A failed
The error message does not tell for which field in the parent type B
the conversion failed.
I don't know how this could be implemented, because the types are constructed bottom-up ...
Right, this may require us to modify the from_dict
generated function to provide some context on parent objects, I don't have a good way of doing that currently either. But let's solve the simple problem first ;-)
one last thing, for the sake of test coverage could you print the error printing in CI? so that we don't decrease the test coverage
Done.
thanks!
MWE