Skip to content

Add option to load args and kwargs explicitly - #33

Open
tuckermcclure wants to merge 1 commit into
mainfrom
tucker/load-args-and-kwargs-explicitly
Open

tuckermcclure wants to merge 1 commit into
mainfrom
tucker/load-args-and-kwargs-explicitly

Conversation

@tuckermcclure

Copy link
Copy Markdown
Member

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:

my_tuple:
  type: Tuple
  values: [1, 2, 3] # <--- There's no way to construct a tuple from "values".

This PR lets us do this:

my_tuple:
  type: Tuple
  args: [1, 2, 3]

or even call a function like so:

my_result:
  type: foo
  args:
    - 1.
    - "Cat"
  kwargs:
    note: "Only one cat today, thanks."

which would obviously call foo(1, "Cat"; note = ...).

However, this is a breaking change, because the args and kwargs are now reserved terms (actually, we use a flexible args_key and kwargs_key for them, just like for types and includes). If you had an old YAML where a struct had a field called kwargs for 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.

@CarlOlsson CarlOlsson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 CarlOlsson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex had some comments, but I approve now.

@tuckermcclure

Copy link
Copy Markdown
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants