Repository: Roger-luo/Configurations.jl (julia)

Issue:
Union{Nothing,String} not allowed?
I wonder why this isn't working, can it be made to work?

```julia
julia> @option struct OptionA
                  name::Union{Nothing,String}=nothing
                  int::Int = 1
              end

julia> Configurations.from_kwargs(OptionA, name="asdf")
ERROR: ArgumentError: invalid key name, possible keys are: int
Stacktrace:
 [1] validate_keywords(::Type{OptionA}; kw::Base.Iterators.Pairs{Symbol, String, Tuple{Symbol}, NamedTuple{(:name,), Tuple{String}}})
   @ Configurations ~/.julia/packages/Configurations/CZY8K/src/Configurations.jl:416
 [2] #from_kwargs#20
   @ ~/.julia/packages/Configurations/CZY8K/src/Configurations.jl:459 [inlined]
 [3] top-level scope
   @ REPL[6]:1
```

(The workaround is to use `Any` as field type.)

Pull request description:
fix #26 and fix #25

Patch:
diff --git a/src/parse.jl b/src/parse.jl
index 70751cb..ec8294e 100644
--- a/src/parse.jl
+++ b/src/parse.jl
@@ -26,8 +26,7 @@ OptionA(;
 """
 function from_dict(::Type{T}, d::AbstractDict{String}; kw...) where T
     # override dict values
-    validate_keywords(T; kw...)
-    from_kwargs!(deepcopy(d), T; kw...)
+    from_underscore_kwargs!(deepcopy(d), T; kw...)
     return from_dict_validate(T, d)
 end
 
@@ -54,24 +53,50 @@ function from_toml_if_exists(::Type{T}, filename::String; kw...) where T
     if isfile(filename)
         return from_toml(T, filename; kw...)
     else
-        return from_kwargs(T;kw...)
+        return from_kwargs(T; kw...)
     end
 end
 
 """
-    from_kwargs(::Type{T}; kw...) where T
+    from_kwargs(convention!, ::Type{T}; kw...) where T
+
+Convert keyword arguments to given option type `T` using `convention!`.
+See also [`from_dict`](@ref).
 
-Convert keyword arguments to given option type `T`. See also [`from_dict`](@ref).
+# Convention
+
+- `from_underscore_kwargs!`: use `_` to disambiguate subfields of the same name, this is the default behaviour.
+- `from_field_kwargs!`: do not disambiguate subfields, errors if there are disambiguity
 """
-function from_kwargs(::Type{T}; kw...) where T
-    validate_keywords(T; kw...)
+function from_kwargs(convention!, ::Type{T}; kw...) where T
     d = OrderedDict{String, Any}()
-    from_kwargs!(d, T; kw...)
+    convention!(d, T; kw...)
     return from_dict_validate(T, d)
 end
 
+"""
+    from_kwargs(::Type{T}; kw...) where T
+
+Convert keyword arguments to given option type `T` using the underscore convention.
+"""
+from_kwargs(::Type{T}; kw...) where T = from_underscore_kwargs(T; kw...)
+
+"""
+    from_underscore_kwargs(::Type{T}; kw...) where T
+
+Convert keyword arguments to given option type `T` using the underscore convention.
+"""
+from_underscore_kwargs(::Type{T}; kw...) where T = from_kwargs(from_underscore_kwargs!, T; kw...)
+
+"""
+    from_field_kwargs(::Type{T}; kw...) where T
+
+Convert keyword arguments to given option type `T` using the field keyword convention.
+"""
+from_field_kwargs(::Type{T}; kw...) where T = from_kwargs(from_field_kwargs!, T; kw...)
+
 function from_dict_validate(::Type{T}, d::AbstractDict{String}) where T
-    is_option(T) || error("$T is not an option type")
+    assert_option(T)
 
     for k in keys(d)
         k == "#filename#" && continue
@@ -185,6 +210,7 @@ function from_dict_inner(::Type{T}, @nospecialize(d)) where T
     return T(args...)
 end
 
+# NOTE: this is for compatibilty
 """
     from_kwargs!(d::AbstractDict{String}, ::Type{T}, prefix::Maybe{Symbol} = nothing; kw...) where T
 
@@ -192,77 +218,145 @@ Internal method for inserting keyword arguments to given dictionary object `d`.
 existing keys in `d` if it is specified by keyword argument.
 """
 function from_kwargs!(d::AbstractDict{String}, ::Type{T}, prefix::Maybe{Symbol} = nothing; kw...) where T
-    if T isa Union
-        from_kwargs!(d, T.a, prefix; kw...)
-        from_kwargs!(d, T.b, prefix; kw...)
-        return d
+    return from_underscore_kwargs!(d, T, prefix; kw...)
+end
+
+function from_underscore_kwargs!(d::AbstractDict{String}, ::Type{T}, prefix::Maybe{Symbol} = nothing; kw...) where T
+    validate_keywords(T, underscore_keywords(T); kw...)
+    unsafe_from_underscore_kwargs!(d, T, prefix; kw...)
+end
+
+function unsafe_from_underscore_kwargs!(d::AbstractDict{String}, ::Type{T}, prefix::Maybe{Symbol} = nothing; kw...) where T
+    return foreach_keywords!(d, T) do name, type
+        key = underscore(prefix, name)
+
+        from_kwargs_option_key!(d, type, name, key, kw) do field_d, field_type
+            unsafe_from_underscore_kwargs!(field_d, field_type, key; kw...)
+        end
     end
+end
 
-    is_option(T) || return
-    fnames = fieldnames(T)
+function from_field_kwargs!(d::AbstractDict{String}, ::Type{T}; kw...) where T
+    validate_keywords(T, field_keywords(T); kw...)
+    unsafe_from_field_kwargs!(d, T; kw...)
+end
 
-    for name in fnames
-        type = fieldtype(T, name)
-        if prefix === nothing
-            key = name
-        else
-            key = Symbol(prefix, :_, name)
+function unsafe_from_field_kwargs!(d::AbstractDict{String}, ::Type{T}; kw...) where T
+    return foreach_keywords!(d, T) do name, type
+        from_kwargs_option_key!(d, type, name, name, kw) do field_d, field_type
+            unsafe_from_field_kwargs!(field_d, field_type; kw...)
         end
+    end
+end
 
-        if is_option(type) || type isa Union
-            field_d = OrderedDict{String, Any}()
-            if haskey(d, string(name)) && d[string(name)] isa AbstractDict
-                field_d = merge!(field_d, d[string(name)])
-            end
+function from_kwargs_option_key!(f, d::AbstractDict, ::Type{T}, name::Symbol, key::Symbol, kw) where T
+    key_str = string(key)
+    name_str = string(name)
+    # shortcut
+    if haskey(kw, key)
+        d[name_str] = kw[key]
+        return d
+    end
+    
+    if is_option(T)
+        field_d = OrderedDict{String, Any}()
+        if haskey(d, name_str)
+            d[name_str] isa AbstractDict || error("option $key_str must be specified using an AbstractDict")
+            field_d = merge!(field_d, d[name_str])
+        end
 
-            from_kwargs!(field_d, type, key; kw...)
-            if !isempty(field_d)
-                d[string(name)] = field_d
-            end
-        elseif haskey(kw, key)
-            d[string(name)] = kw[key]
+        f(field_d, T) # recurse into subfields
+        if !isempty(field_d)
+            d[name_str] = field_d
         end
+    elseif T isa Union
+        from_kwargs_option_key!(f, d, T.a, name, key, kw)
+        from_kwargs_option_key!(f, d, T.b, name, key, kw)
     end
     return d
 end
 
-function validate_keywords(::Type{T}; kw...) where T
-    ks = keywords(T)
-    hint = join(map(x->LIGHT_BLUE_FG(string(x)), ks), ", ")
+function validate_keywords(::Type{T}, keys = underscore_keywords(T); kw...) where T
+    if length(keys) > 8
+        hint = join(map(x->LIGHT_BLUE_FG(string(x)), keys[1:8]), ", ")
+        hint *= "... please check documentation for other valid keys"
+    else
+        hint = join(map(x->LIGHT_BLUE_FG(string(x)), keys), ", ")
+    end
+
     for (k, v) in kw
-        k in ks || throw(ArgumentError("invalid key $(LIGHT_BLUE_FG(string(k))), possible keys are: $hint"))
+        k in keys || throw(ArgumentError("invalid key $(LIGHT_BLUE_FG(string(k))), possible keys are: $hint"))
     end
     return
 end
 
 """
-    keywords(::Type{T}) where T -> Vector{Symbol}
+    field_keywords(::Type{T}) where T
 
-Get all the keywords of type `T`.
+Return all the option type field names given `T`, error if there are duplicated sub-fields.
 """
-keywords(::Type{T}) where T = collect_keywords!(Symbol[], T)
+function field_keywords(::Type{T}) where T
+    return collect_field_keywords!(Symbol[], T)
+end
 
-function collect_keywords!(list::Vector{Symbol}, ::Type{T}, prefix::Maybe{Symbol} = nothing) where T
-    if T isa Union
-        collect_keywords!(list, T.a, prefix)
-        collect_keywords!(list, T.b, prefix)
-        return list
-    end
+"""
+    underscore_keywords(::Type{T}) where T
+
+Return keywords given `T` using the underscore convention.
+"""
+function underscore_keywords(::Type{T}) where T
+    return collect_underscore_keywords!(Symbol[], T)
+end
 
+function foreach_keywords!(f, list, ::Type{T}) where T
     is_option(T) || return list
+
     for name in fieldnames(T)
         type = fieldtype(T, name)
-        if prefix === nothing
-            key = name
+        f(name, type)
+    end
+    return list
+end
+
+function underscore(prefix::Maybe{Symbol}, name)
+    if prefix === nothing
+        return name
+    else
+        return Symbol(prefix, :_, name)
+    end
+end
+
+function collect_field_keywords!(list::Vector{Symbol}, ::Type{T}) where T
+    return foreach_keywords!(list, T) do name, type
+        msg = "duplicated field $(LIGHT_BLUE_FG(string(name))) " *
+            "in type $(GREEN_FG(string(T))) and its sub-fields"
+        if is_option(type)
+            collect_field_keywords!(list, type)
+        elseif type isa Union
+            name in list && error(msg)
+            push!(list, name)
+            # recurse into Union
+            collect_field_keywords!(list, type.a)
+            collect_field_keywords!(list, type.b)
         else
-            key = Symbol(prefix, :_, name)
+            name in list && error(msg)
+            push!(list, name)
         end
+    end
+end
 
-        if is_option(type) || type isa Union
-            collect_keywords!(list, type, key)
+function collect_underscore_keywords!(list::Vector{Symbol}, ::Type{T}, prefix::Maybe{Symbol} = nothing) where T
+    return foreach_keywords!(list, T) do name, type
+        key = underscore(prefix, name)
+
+        if is_option(type)
+            collect_underscore_keywords!(list, type, key)
+        elseif type isa Union
+            push!(list, key)
+            collect_underscore_keywords!(list, type.a, key)
+            collect_underscore_keywords!(list, type.b, key)
         else
             push!(list, key)
         end
     end
-    return list
 end


Test patch:
diff --git a/test/runtests.jl b/test/runtests.jl
index 7796351..d3bedaf 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -18,6 +18,34 @@ end
     float::Float64 = 0.3
 end
 
+@option struct OptionC
+    num::Float64
+
+    function OptionC(num::Float64)
+        num > 0 || error("not positive")
+        new(num)
+    end
+end
+
+@option struct OptionD
+    opt::Union{OptionA, OptionB}
+end
+
+@option struct OptionE
+    field::Union{Nothing, OptionA} = nothing
+end
+
+@option struct OptionF
+    name::Union{Nothing,String}=nothing
+    int::Int = 1
+end
+
+@option struct OptionG
+    opt::OptionF = OptionF()
+    name::String = "ABC"
+end
+
+
 dict1 = OrderedDict{String, Any}(
     "opt" => OrderedDict{String, Any}(
         "name" => "Roger",
@@ -88,14 +116,31 @@ end
 
 @testset "from_kwargs" begin
     @test Configurations.from_kwargs(OptionB; opt_name="Roger", opt_int=2, float=0.33) == option
-end
 
-@option struct OptionC
-    num::Float64
+    @test Configurations.from_kwargs(OptionG;name="AAA", opt_name="Roger") == OptionG(;
+        opt = OptionF(;
+            name = "Roger",
+        ),
+        name = "AAA",
+    )
 
-    function OptionC(num::Float64)
-        num > 0 || error("not positive")
-        new(num)
+    d = Configurations.from_kwargs!(OrderedDict{String, Any}(), OptionG; name="AAA", opt_name="Roger")
+    @test d["name"] == "AAA"
+    @test d["opt"]["name"] == "Roger"
+
+    # error for wrong specification when we can't overwrite
+    d = OrderedDict{String, Any}("opt"=>"AAA")
+    @test_throws ErrorException Configurations.from_kwargs!(d, OptionG; name="AAA")
+
+    @testset "from_field_kwargs" begin
+        # error for ambiguious keyword
+        @test_throws ErrorException Configurations.from_field_kwargs(OptionG;name="AAA")
+
+        @test Configurations.from_field_kwargs(OptionB;name="AAA") == OptionB(;
+            opt = OptionA(;
+                name = "AAA",
+            ),
+        )
     end
 end
 
@@ -103,10 +148,6 @@ end
     @test_throws ErrorException OptionC(-1.0)
 end
 
-@option struct OptionD
-    opt::Union{OptionA, OptionB}
-end
-
 @testset "test multi option type" begin
     d1 = OrderedDict{String, Any}(
         "opt" => OrderedDict{String, Any}(
@@ -139,10 +180,6 @@ end
     @test_throws ErrorException from_dict(OptionD, d3)
 end
 
-@option struct OptionE
-    field::Union{Nothing, OptionA} = nothing
-end
-
 @testset "optional field" begin
     d = OrderedDict{String, Any}(
         "float" => 0.33
@@ -161,8 +198,21 @@ end
     )
 end
 
+@option struct LongValidateErrorHint
+    x1
+    x2
+    x3
+    x4
+    x5
+    x6
+    x7
+    x8
+    x9
+end
+
 @testset "validate keys" begin
     @test_throws ArgumentError Configurations.validate_keywords(OptionA; abc=2)
+    @test_throws ArgumentError Configurations.validate_keywords(LongValidateErrorHint; abc=2)
     @test Configurations.validate_keywords(OptionB; opt_name="AAA") === nothing
     @test_throws ArgumentError Configurations.validate_keywords(OptionB; opt_abc="AAA")
 end