Roger-luo / Configurations.jl

Options & Configurations made easy.
https://configurations.rogerluo.dev/stable
MIT License
80 stars 12 forks source link

Interpolate `Reflect` in `from_dict_generated` #94

Closed torfjelde closed 1 year ago

torfjelde commented 1 year ago

Currently, defining options in a module without making Reflect explicitly available in the scope breaks from_dict due to the lack of interpolation of Reflect in https://github.com/Roger-luo/Configurations.jl/blob/1dedfb37d5ed61aa45e62e1fa4230859877feddd/src/from_dict.jl#L338-L375

torfjelde commented 1 year ago

Okay, I now realized this is probably because @option can't handle Reflect unless it's explicitly of the form Reflect or Configurations.Reflect :confused:

torfjelde commented 1 year ago

On second thought, it seems one can easily avoid Reflect not being available by the aforementioned references by simply providing the default value by hand :+1:

Roger-luo commented 1 year ago

I'm wondering if you have an example to describe the behaviour you are expecting? IIRC, the reason why it's not interpolated in some places is because it is supposed to refer whatever user brought into the namespace. But I could miss checking a few places if this is right.

torfjelde commented 1 year ago

I ran into the issue in a situation similar to this:

julia> module A
       using Configurations
       end

julia> A.@option struct MyOptions
           type::A.Reflect=A.Reflect()
       end

julia> d = A.to_dict(MyOptions())
OrderedCollections.OrderedDict{String, Any} with 1 entry:
  "type" => "Main.MyOptions"

julia> A.from_dict(MyOptions, d)
ERROR: UndefVarError: `Reflect` not defined
Stacktrace:
 [1] macro expansion
   @ ~/.julia/packages/Configurations/jwESn/src/from_dict.jl:360 [inlined]
 [2] macro expansion
   @ ~/.julia/packages/Configurations/jwESn/src/codegen.jl:0 [inlined]
 [3] from_dict_specialize(#unused#::Type{MyOptions}, d::OrderedCollections.OrderedDict{String, Any})
   @ Main ~/.julia/packages/Configurations/jwESn/src/codegen.jl:362
 [4] #from_dict#4
   @ ~/.julia/packages/Configurations/jwESn/src/from_dict.jl:46 [inlined]
 [5] from_dict(::Type{MyOptions}, d::OrderedCollections.OrderedDict{String, Any})
   @ Configurations ~/.julia/packages/Configurations/jwESn/src/from_dict.jl:33
 [6] top-level scope
   @ REPL[17]:1
torfjelde commented 1 year ago

IRC, the reason why it's not interpolated in some places is because it is supposed to refer whatever user brought into the namespace.

That makes sense, but in this scenario it's not so clear to me why you'd want Reflect to refer to whichever Reflect rather than Configurations.Reflect :confused:

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (1dedfb3) 96.76% compared to head (5c0ce78) 96.76%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #94 +/- ## ======================================= Coverage 96.76% 96.76% ======================================= Files 12 12 Lines 618 618 ======================================= Hits 598 598 Misses 20 20 ``` | [Files Changed](https://app.codecov.io/gh/Roger-luo/Configurations.jl/pull/94?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Xiu-zhe+%28Roger%29+Luo) | Coverage Δ | | |---|---|---| | [src/from\_dict.jl](https://app.codecov.io/gh/Roger-luo/Configurations.jl/pull/94?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Xiu-zhe+%28Roger%29+Luo#diff-c3JjL2Zyb21fZGljdC5qbA==) | `93.51% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Roger-luo commented 1 year ago

ah... You are right, we should interpolate this one here.

Roger-luo commented 1 year ago

do you mind putting the above example you provide in the test? then we are good to merge

torfjelde commented 1 year ago

Added tests now:)

torfjelde commented 1 year ago

Bump :grimacing:

Roger-luo commented 1 year ago

Oops somehow didn't see it. LGTM! thank you!

torfjelde commented 1 year ago

Awesome; thanks @Roger-luo !