Repository navigation
Add option to load args and kwargs explicitly - #33
tuckermcclure wants to merge 1 commit into
Conversation
CarlOlsson
left a comment
There was a problem hiding this comment.
Codex review:
[P2] Specialized dictionary decoders bypass explicit construction
The new explicit-call check in the generic from_dict(::Type{T}, ::AbstractDict) method is not reached when a more-specific decoder exists for the target type. This affects types including Rational, Complex, Dict, NamedTuple, and Matrix.
For example:
PortableStructs.from_dict(
Rational,
OrderedDict("args" => Any[1, 2]);
type_key = "type",
base_module = Main,
)This should return 1//2, but the existing Rational decoder runs first, tries to read "num", and throws KeyError.
It also fails when nested inside a concretely typed field, even with a tag:
value:
type: Rational
args: [1, 2]The same tagged value succeeds when loaded at the top level as Any, so behavior depends unexpectedly on the caller's expected type. Explicit-call detection needs to occur before specialized dictionary decoding, with regression tests for a specialized target and a nested typed field.
[P3] The PR description's Tuple example does not work
The example
type: Tuple
args: [1, 2, 3]results in Tuple(1, 2, 3), which is not a valid Julia call. The README and tests correctly use type: Core.tuple. The PR description should use that spelling, or Tuple needs special handling.
Local Pkg.test(), GitHub CI, and git diff --check pass.
CarlOlsson
left a comment
There was a problem hiding this comment.
Codex had some comments, but I approve now.
|
I'm going to allow this to sit here since it's a breaking change. We have some smaller changes we should merge and release first. |
Right now, it's actually hard to construct something that only uses positional constructors (or call a function that only uses these). E.g., you can't actually load this from YAML:
This PR lets us do this:
or even call a function like so:
which would obviously call
foo(1, "Cat"; note = ...).However, this is a breaking change, because the
argsandkwargsare now reserved terms (actually, we use a flexibleargs_keyandkwargs_keyfor them, just like for types and includes). If you had an old YAML where a struct had a field calledkwargsfor some reason, that would work in the prior version but not with this change.Note also that this is a read-only option. Nothing writes to YAML by counting on positional arguments. However, moving forward, we could explicitly write some types to YAML this way.
I like how clean this implementation is.