Skip to content

Add to_tree for extendable type writing - #75

Open
cgarling wants to merge 4 commits into
mainfrom
write-converter
Open

cgarling wants to merge 4 commits into
mainfrom
write-converter

Conversation

@cgarling

@cgarling cgarling commented Sep 2, 2026

Copy link
Copy Markdown
Member

This adds a public write-conversion API (to_tree) so arbitrary Julia objects can be embedded in ASDF documents.

Packages extend ASDF.to_tree(value, context), while ASDF.jl recursively converts returned nodes, preserves tags and ordering, detects cycles, and routes arrays through the existing block writer.

This establishes the write-side foundation for future extension support. Unknown tags can already be loaded as generic tagged nodes with extensions = true; read-side object reconstruction, schema validation, and extension provenance can build on this protocol later.

Adds a write-conversion protocol for packages to extend allowing arbitrary Julia types to be embedded into ASDF documents.
@codecov

codecov Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 99.36%. Comparing base (cfbde37) to head (50985e6).

Files with missing lines Patch % Lines
src/ASDF.jl 97.36% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main      #75      +/-   ##
===========================================
- Coverage   100.00%   99.36%   -0.64%     
===========================================
  Files            1        1              
  Lines          589      626      +37     
===========================================
+ Hits           589      622      +33     
- Misses           0        4       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@icweaver
icweaver self-requested a review September 4, 2026 00:20

@icweaver icweaver 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.

Hi Chris, thanks for this much needed PR! It adds a good bit of functionality, but I must admit that the added complexity has moved this review pretty far out of my area of expertise. To that end, I spent the last few days going back-and-forth with claude, and settled on this first pass at a review for you to take a look at.

Review - 46c4bde

Code review: write-converter branch

Commit under review: 46c4bde "Add to_tree for extendable type writing" (one commit on top of main, working tree clean)
Diff: git diff main...write-converter, 7 files, +316 / −2
Date: 2026-09-10
Model: Fable 5.1 (ultracode)

Scope and method

The branch adds a write-side conversion hook (ASDF.to_tree), an opaque ASDF.WriteContext, recursive tree conversion with cycle detection, a new docs page, and test/test-write-converters.jl.

Baseline on this branch, before looking for problems:

  • The new tests, Aqua, and doctests all pass.
  • A main-vs-branch round-trip on a broad document (plain scalars, floats, nested dicts, vectors, tagged nodes, binary blocks) produces byte-identical files, so the change does not alter existing output.

Every finding below is therefore a case the tests do not cover. Each code snippet is self-contained: run it as-is from the repository's test project environment (julia --project=test), which provides ASDF.jl and OrderedCollections. Output shown in comments is what Julia 1.13.0 printed when each snippet was run from a fresh session. Findings were each checked by an independent verifier that tried to refute them; the three highest-impact ones were additionally reproduced a second time on Julia 1.12 before this document was written. Line numbers link to the reviewed commit.

Summary

# Severity Location Finding
1 High src/ASDF.jl:1203 Hook output is never re-dispatched through to_tree; a hook returning another hooked object writes string(val) into the file silently
2 High src/ASDF.jl:1186 A hook with an unannotated context argument is an ambiguity MethodError on every call
3 Medium src/ASDF.jl:1202 Self-tagging AbstractDict/AbstractVector subtype throws a false "cyclic" error
4 Medium src/ASDF.jl:1187 A wrong-arity one-argument hook works at the REPL but is silently ignored by write_file
5 Medium src/ASDF.jl:1207 Walker skips Tuple/NamedTuple/Pair/AbstractSet and never validates leaves; a Pair produces an unloadable file
6 Medium src/ASDF.jl:1232 map keeps a Matrix a Matrix; N-d arrays are written as text and the new test cements it
7 Medium src/ASDF.jl:1870 to_tree then yaml_compliant rebuilds the whole tree twice, roughly doubling pre-write cost
8 Design src/ASDF.jl:1165 WriteContext is public but carries only private cycle-guard state; a one-argument hook plus a private walker is the minimal form and removes findings 2 and 4
9 Low src/ASDF.jl:1200 A hook that recursively calls to_tree on cyclic input overflows the stack instead of raising the documented ArgumentError
10 Low src/ASDF.jl:1226 Mapping keys are never passed through the converter
11 Low src/ASDF.jl:1189 Cycle bookkeeping is spread over five sites; the TaggedScalar method is dead code
12 Low docs/src/api.md:24 Hand-maintained Filter mirrors the manual @docs list; both are deletable
13 Low docs/src/custom-types.md:16 Example uses an unordered Dict, so the written key order is reversed from the source
14 Low test/test-write-converters.jl:60 Abstract-type fixtures and the save block test Julia dispatch, not ASDF
15 Low src/ASDF.jl:1064 The Tagged* docstring never says the tag field must be the full URI

Recommended direction

Findings 2, 4, 8, and 11 share one root cause: the public surface splits into a two-argument hook and a one-argument walker, joined by a WriteContext that no hook can actually use. A patched copy of the branch was built and tested with this shape instead (a sketch of the replaced internals in src/ASDF.jl, not a standalone snippet):

# The hook. Packages extend this one method.
to_tree(value) = value

# Private walker; `active` is the cycle guard.
function _convert_tree(value, active = Base.IdSet{Any}())
    value in active && throw(ArgumentError("cyclic ASDF write conversion involving $(typeof(value)) is not supported"))
    push!(active, value)
    try
        return _convert_tree_children(to_tree(value), active)
    finally
        delete!(active, value)
    end
end

_convert_tree_children(value, active) = value
_convert_tree_children(v::TaggedMapping, active) = TaggedMapping(v.tag, _convert_tree_children(v.value, active))
_convert_tree_children(v::TaggedSequence, active) = TaggedSequence(v.tag, _convert_tree_children(v.value, active))
_convert_tree_children(v::AbstractDict, active) = OrderedDict{Any, Any}(k => _convert_tree(item, active) for (k, item) in v)
_convert_tree_children(v::AbstractArray, active) = map(item -> _convert_tree(item, active), v)

With WriteContext deleted (48 lines fewer overall) the full Pkg.test() passes on Julia 1.10.12 and 1.12.7. The untyped-context hook problem disappears because there is no context argument. A mistaken hook arity becomes visible at the REPL. The single guard catches 9/9 cycle cases including the isbits CyclicWriteValue, flags 0/7 DAG (shared, non-cyclic) inputs, and converts the self-tagging AbstractDict subtype from finding 3 correctly. Base.IdSet is available on Julia 1.10 and wraps the same IdDict{Any, Nothing} the branch uses.

If a context is genuinely needed later (schema selection, array-storage policy), it can be added additively as to_tree(value, context) = to_tree(value) without breaking anyone. Under the project's alpha policy this is a routine minor-version change either way.

Findings 1, 5, 6, 7, 9, and 10 are independent of that choice and are discussed individually below.


Correctness

1. Hook output is never re-dispatched through to_tree — High

src/ASDF.jl:1203

_convert_tree calls the hook once and passes the result straight to _convert_tree_children(converted, context). The result never re-enters to_tree. A hook that delegates to another hooked type, or that returns a hooked AbstractDict subtype, therefore leaves that object unconverted, and YAML.jl's fallback writes string(val) into the file with no error.

using ASDF, OrderedCollections

struct Kelvin;  k::Float64; end
struct Celsius; c::Float64; end
ASDF.to_tree(k::Kelvin,  ::ASDF.WriteContext) = ASDF.TaggedMapping("tag:example.org/kelvin-1.0.0", OrderedDict("k" => k.k))
ASDF.to_tree(c::Celsius, ::ASDF.WriteContext) = Kelvin(c.c + 273.15)

ASDF.to_tree(Celsius(20.0))
# Kelvin(293.15)                 not a TaggedMapping: the Kelvin hook never ran

f = tempname()
ASDF.write_file(f, OrderedDict("t" => Celsius(20.0)))   # no error; the file contains  t: Kelvin(293.15)
ASDF.load_file(f)["t"]
# "Kelvin(293.15)"

An in-contract variant: a hook returns a custom AbstractDict subtype that has its own tagging hook. _convert_tree_children(::AbstractDict) iterates it as a plain mapping and the tag is silently dropped, while the same object placed as a child keeps its tag.

Suggested fix. Re-enter _convert_tree(converted, ctx) when converted !== value. Note that a naive re-dispatch turns a hook that returns a fresh object of its own type into unbounded recursion (this wedged Julia 1.12 during verification), so guard with typeof(converted) === typeof(value) or a depth limit, or alternatively make the leaf fallback error on types the writer does not support (see finding 5).

2. Unannotated context argument is an ambiguity MethodError — High

src/ASDF.jl:1186

The fallback is to_tree(value, ::WriteContext) = value, with signature (Any, WriteContext). The idiomatic hook a package author will write, given that the docs say to treat the context as opaque, is (Measurement, Any). Neither method is more specific than the other.

using ASDF, OrderedCollections

struct Meas; v::Float64; end
ASDF.to_tree(m::Meas, context) = ASDF.TaggedMapping("tag:example.org/meas-1.0.0", OrderedDict("v" => m.v))

ASDF.to_tree(Meas(1.0))
# ERROR: MethodError: to_tree(::Meas, ::ASDF.WriteContext) is ambiguous.

write_file fails the same way.

Suggested fix. Change the fallback to to_tree(value, context) = value, an (Any, Any) method. The typed hooks in the tests still pass and Test.detect_ambiguities(ASDF) stays empty. The recommended direction above removes the argument entirely.

3. Self-tagging container subtype throws a false "cyclic" error — Medium

src/ASDF.jl:1202

_convert_tree registers value in the active set, then _convert_tree_children(::TaggedMapping) (line 1211) or (::TaggedSequence) (line 1218) registers converted.value. When a hook on an AbstractDict or AbstractVector subtype wraps itself, those are the same object.

using ASDF, OrderedCollections

struct Header <: AbstractDict{String, Any}
    d::OrderedDict{String, Any}
end
Base.iterate(h::Header, state...) = iterate(h.d, state...)
Base.length(h::Header) = length(h.d)
Base.getindex(h::Header, k) = h.d[k]
Base.get(h::Header, k, default) = get(h.d, k, default)
ASDF.to_tree(h::Header, ::ASDF.WriteContext) = ASDF.TaggedMapping("tag:example.org/header-1.0.0", h)

h = Header(OrderedDict{String, Any}("a" => 1))
ASDF.to_tree(h)
# ERROR: ArgumentError: cyclic ASDF write conversion involving Header is not supported

# Wrapping a copy instead of `h` itself works, so there is no real cycle:
ASDF.to_tree(ASDF.TaggedMapping("tag:example.org/header-1.0.0", Dict(h)))
# ASDF.TaggedMapping{OrderedDict{Any, Any}}("a" => 1)

# Same for a vector subtype wrapped in a TaggedSequence:
struct MyVec <: AbstractVector{Any}; v::Vector{Any}; end
Base.size(x::MyVec) = size(x.v)
Base.getindex(x::MyVec, i::Int) = x.v[i]
ASDF.to_tree(x::MyVec, ::ASDF.WriteContext) = ASDF.TaggedSequence("tag:example.org/myvec-1.0.0", x)

ASDF.to_tree(MyVec(Any[1]))
# ERROR: ArgumentError: cyclic ASDF write conversion involving MyVec is not supported

Calling _convert_tree_children on the converted node directly also terminates, so the guard, not the data, is at fault.

Suggested fix. A single cycle guard in _convert_tree (finding 11) removes the double registration.

4. Wrong-arity hook works at the REPL but is ignored by write_file — Medium

src/ASDF.jl:1187

The public one-argument walker to_tree(value) shares its generic function with the two-argument hook. A package author who defines the wrong arity gets a working REPL call, because their more specific method shadows the walker, while write_file only ever calls the two-argument form.

using ASDF, OrderedCollections

struct Oops; n::Int; end
# Wrong arity: this defines a one-argument method, which is the walker, not the hook.
ASDF.to_tree(o::Oops) = ASDF.TaggedMapping("tag:example.org/oops-1.0.0", OrderedDict("n" => o.n))

ASDF.to_tree(Oops(1))
# ASDF.TaggedMapping{OrderedDict{String, Int64}}("n" => 1)      looks correct at the REPL

f = tempname()
ASDF.write_file(f, OrderedDict("o" => Oops(1)))   # no error; the file contains  o: Oops(1)
ASDF.load_file(f)["o"]
# "Oops(1)"

Neither Aqua nor Julia warns about the extra arity.

Suggested fix. Give the walker a distinct or private name, or make the leaf fallback reject non-writer-supported types with an error naming ASDF.to_tree(::T, ::WriteContext). The recommended direction does the former.

5. Walker skips Tuple/NamedTuple/Pair/AbstractSet and never validates leaves — Medium

src/ASDF.jl:1207

The generic fallback _convert_tree_children(value, context) = value does not descend into these containers, so hooked objects inside them are never converted and cycles through them go undetected. It also does not validate leaves, so unsupported values reach YAML.jl's string(val) fallback.

using ASDF, OrderedCollections

struct Q; n::Int; end
ASDF.to_tree(q::Q, ::ASDF.WriteContext) = ASDF.TaggedMapping("tag:example.org/q-1.0.0", OrderedDict("n" => q.n))
struct NoHook; n::Int; end   # no to_tree method

doc = OrderedDict(
    "t"   => (Q(1), 2),
    "nt"  => (q = Q(1),),
    "s"   => Set([Q(1)]),
    "p"   => ("a" => 1),
    "raw" => NoHook(7),
)
f = tempname()
ASDF.write_file(f, doc)   # succeeds
print(read(f, String))
# ...
# t: (Q(1), 2)
# nt: (q = Q(1),)
# s: Set(Q[Q(1)])
# p:   a: 1
# raw: NoHook(7)
# ...

ASDF.load_file(f)
# ERROR: while parsing a block mapping at line 8, column 0: expected <block end>,
#        but found YAML.BlockMappingStartToken at line 11, column 6          (the Pair line)

delete!(doc, "p")
ASDF.write_file(f, doc)
af = ASDF.load_file(f)
af["t"], af["nt"], af["s"], af["raw"]
# ("(Q(1), 2)", "(q = Q(1),)", "Set(Q[Q(1)])", "NoHook(7)")                 all Strings

The unvalidated leaf and the Pair output are byte-identical on main, so that part is pre-existing. What is new is the prose: README.md:13, custom-types.md:4, and index.md:129 now promise objects convert "anywhere" in a document, while the docstrings correctly say only "mappings, arrays, or tagged nodes".

Suggested fix. Either descend into these containers, error on unsupported leaves, or narrow the prose to match the docstrings.

6. map keeps a Matrix a Matrix; N-d arrays written as text — Medium

src/ASDF.jl:1232

_convert_tree_children(::AbstractArray) uses map, which preserves shape. YAML.jl only serializes AbstractVector, so an unwrapped N-d array is written as its show text.

using ASDF, OrderedCollections

f = tempname()
ASDF.write_file(f, OrderedDict("m" => [1 2; 3 4]))   # the file contains  m: [1 2; 3 4]
ASDF.load_file(f)["m"]
# ["1 2; 3 4"]                                        identical on main

struct Q; n::Int; end
ASDF.to_tree(q::Q, ::ASDF.WriteContext) = ASDF.TaggedMapping("tag:example.org/q-1.0.0", OrderedDict("n" => q.n))

ASDF.write_file(f, OrderedDict("qv" => [Q(1), Q(2)], "qm" => [Q(1) Q(2)]))
print(read(f, String))
# ...
# qv:
#   - !<tag:example.org/q-1.0.0>
#     n: 1
#   - !<tag:example.org/q-1.0.0>
#     n: 2
# qm: ASDF.TaggedMapping{OrderedDict{Any, Any}}[ASDF.TaggedMapping("n" => 1) ASDF.TaggedMapping("n" => 2)]
# ...

af = ASDF.load_file(f; extensions = true)
af["qv"]   # 2-element Vector of TaggedMapping, as expected
af["qm"]   # one String: "ASDF.TaggedMapping{OrderedDict{Any, Any}}[ASDF.TaggedMapping(\"n\" => 1) ...]"

The plain-Matrix case is byte-identical on main. The new surface is a Matrix of hooked objects: the hook runs on every element, map returns a Matrix{TaggedMapping}, and that is written as its show text, while the same values in a Vector write correctly. The new test at test/test-write-converters.jl:73 (@test plain["matrix"] == source["matrix"]) asserts the pass-through as intended behaviour.

Suggested fix. The new walk is the natural place to reject non-vector arrays, or to route them through NDArrayWrapper(...; inline = true), rather than cementing the current behaviour in a test.

7. to_tree then yaml_compliant rebuilds the whole tree twice — Medium (efficiency)

src/ASDF.jl:1870

write_file now runs two structurally identical full-tree rebuilds back to back, and the new walk reallocates every container even when nothing changed.

Document to_tree yaml_compliant YAML.write
20k small dicts 28–34 ms 31–33 ms 29 ms

So the new pass adds roughly 50% to write_file. On 100k dicts, yaml_compliant(to_tree(doc)) takes 430–520 ms / 186 MiB versus 137–186 ms / 100–115 MiB for one guarded walk that does both. A copy-on-write walker that returns value when every child is === unchanged drops the no-custom-object case from 182 ms / 77 MiB to 22 ms / 10 MiB, and a 1e6-element inline Vector{Float64} is no longer copied twice. The OrderedDict{Any, Any}(k => f(v) for ...) generator at lines 1212 and 1226 (and pre-existing 1265) is also 3–4x slower than a sizehint! plus setindex! loop.

Suggested fix. Fold the float rule into the walker as _convert_tree_children(v::AbstractFloat, ctx) = YAMLScalar(yaml_float_string(v)) and call one walker from write_file.

9. Recursive hook on cyclic input overflows the stack — Low

src/ASDF.jl:1200

The hook runs before value is marked active (and only on the converted !== value branch), and the public one-argument to_tree always starts a fresh WriteContext. A hook that recursively converts a child therefore turns cyclic input into a StackOverflowError instead of the documented ArgumentError.

using ASDF, OrderedCollections

mutable struct Node; next; end
a = Node(nothing); a.next = a

# A hook that recurses through ASDF itself instead of returning the child:
ASDF.to_tree(n::Node, ::ASDF.WriteContext) =
    OrderedDict("next" => n.next === nothing ? nothing : ASDF.to_tree(n.next))

ASDF.to_tree(a)
# Warning: detected a stack overflow; program state may be corrupted, so further execution might be unreliable.
# ERROR: StackOverflowError                          segfaulted outright in one Julia 1.12 run

# The shallow form the docs describe is detected correctly (best run in a fresh session after the overflow):
ASDF.to_tree(n::Node, ::ASDF.WriteContext) = OrderedDict("next" => n.next)
ASDF.to_tree(a)
# ERROR: ArgumentError: cyclic ASDF write conversion involving Node is not supported

The same happens if the hook calls the private _convert_tree(n.next, ctx) instead. Low severity because the docs say hooks are shallow, but the one-argument form is advertised for "inspecting".

Suggested fix. Add a doc caveat. Wrapping only the hook call in _with_active fixed the threaded-context variant with zero test regressions.

10. Mapping keys are never passed through the converter — Low

src/ASDF.jl:1226

Keys are copied verbatim (key => _convert_tree(item, context), also at line 1212), so a custom key with a hook is written via YAML.jl's string(pair[1]).

using ASDF, OrderedCollections

struct K; n::Int; end
ASDF.to_tree(k::K, ::ASDF.WriteContext) = "k$(k.n)"

ASDF.to_tree(OrderedDict(K(1) => K(2)))
# OrderedDict{Any, Any}(K(1) => "k2")                 value converted, key not

f = tempname()
ASDF.write_file(f, OrderedDict(K(1) => K(2)))          # the file contains  K(1): "k2"
first(ASDF.load_file(f).metadata)
# "K(1)" => "k2"

This is pre-existing (main's yaml_compliant(::AbstractDict) has the same shape) and ASDF restricts keys to bool/int/str.

Suggested fix. A one-line doc note ("keys are not converted") is proportionate.


Design and simplification

8. WriteContext is public but carries only private state — Design

src/ASDF.jl:1165

context._active is read only inside _with_active (lines 1190–1195). Every hook in the tests and docs names context and never uses it, and there is no context-threaded recursive entry point a hook could pass it to. The two-argument-hook plus one-argument-walker split it forces is the root cause of findings 2 and 4. See Recommended direction above for the tested alternative.

11. Cycle bookkeeping is spread over five sites; dead TaggedScalar method — Low

src/ASDF.jl:1189

The guard lives in _with_active, in the two-branch _convert_tree, and in a _with_active wrapper inside each of the four container methods, with an asymmetric value.value key for the Tagged* wrappers. A single guard inlined in _convert_tree plus four one-liners (the same shape as yaml_compliant at lines 1263–1266) is equivalent, about 25 lines shorter, and incidentally fixes finding 3. That version passes all 38 tests, catches 9/9 cycle cases including the isbits CyclicWriteValue, and flags 0/7 DAG inputs.

_convert_tree_children(::TaggedScalar) at line 1208 is dead code: TaggedScalar is neither an AbstractDict nor an AbstractArray, so it duplicates the fallback at line 1207. Deleting it leaves the WriteScalar test and the round-trip passing.

12. Hand-maintained Filter mirrors the manual @docs list — Low

docs/src/api.md:24

Filter = value -> value ∉ (…5 names…) exists only to suppress Documenter's duplicate-docs error caused by the new manual @docs block at lines 12–18 that lists the same five names. Two lists that must stay in sync; each future write-side name needs edits in both or the build fails.

A scratch Documenter build with the "Custom type writing" section and the Filter line both removed builds strictly (exit 0, no docerrors), and every [ASDF.to_tree](@ref), [ASDF.WriteContext](@ref) and [ASDF.TaggedMapping](@ref) link in custom-types.md and index.md still resolves, with the targets back in the Private section as on main.

The diff also repeats the to_tree contract prose: custom-types.md:50–61 closely paraphrases the docstring at src/ASDF.jl:1176–1184, and README.md:13, index.md:128, and custom-types.md:4 carry the same sentence.


Documentation and tests

13. Example uses an unordered Dict, so written key order is reversed — Low

docs/src/custom-types.md:16

The worked example builds the TaggedMapping payload with Dict("value" => ..., "unit" => ...). Running it verbatim gives keys ["unit", "value"] (hash order) and the written YAML has unit: "s" before value: 1200.0. The tests, write_file, and the TaggedMapping docstring all standardise on OrderedDict for deterministic key order, and neither custom-types.md nor the README mentions order, so users copying the example get nondeterministic layouts from a package that went to some length to preserve order.

using ASDF, OrderedCollections

struct Measurement
    value::Float64
    unit::String
end
# As written in docs/src/custom-types.md, with a plain Dict:
ASDF.to_tree(m::Measurement, ::ASDF.WriteContext) =
    ASDF.TaggedMapping("tag:example.org/measurement-1.0.0", Dict("value" => m.value, "unit" => m.unit))

collect(keys(ASDF.to_tree(Measurement(1200.0, "s"))))
# Any["unit", "value"]                                 hash order, reversed from the source

f = tempname()
ASDF.write_file(f, OrderedDict("exposure" => Measurement(1200.0, "s")))
print(read(f, String))
# ...
# exposure:
#   !<tag:example.org/measurement-1.0.0>
#   unit: "s"
#   value: 1200.0
# ...

Suggested fix. Use OrderedDict in the example. Also fix the inaccurate half of the comment at src/ASDF.jl:1869: blocks are collected during YAML.write (line 1781), not in a pre-write pass.

14. Abstract-type fixtures and save block test Julia dispatch, not ASDF — Low

test/test-write-converters.jl:60

  • AbstractWriteValue/SpecialWriteValue and the asserts at lines 59–60 only check that Julia picks the more specific method. ASDF has no dispatch registry; _convert_tree just calls to_tree(value, context). Line 59 duplicates line 45 and line 60 duplicates line 107.
  • Lines 38–40 ("shallow") call the test's own two-argument method, not ASDF code.
  • The save block at lines 114–118 re-tests a one-line delegation already covered by test-write.jl:20/62/69; only the value assert at line 118 is new and can move into the write_file block.
  • Lines 111–112 duplicate the non-mutation check at line 76, since OrderedDict{Any, Any}(doc)["roman"] === doc["roman"].
  • The new tag:example.org/write/... namespace diverges from the existing tag:example.org:mylib/... fixtures.

Collapsing to one WriteValue, dropping the abstract type, and deleting lines 114–118 and 111–112 loses no ASDF coverage (38/38 pass either way).

15. Tagged* docstring never says the tag must be the full URI — Low (plausible)

src/ASDF.jl:1064

The shared TaggedMapping/TaggedSequence/TaggedScalar docstring is now rendered as the API reference for to_tree return values (api.md:12–18, custom-types.md:54–58), but still describes them only as load-side products and mentions tags in !core/... shorthand.

using ASDF, OrderedCollections

f = tempname()
sw = OrderedDict("name" => "x", "version" => "1")

ASDF.write_file(f, OrderedDict("sw" => ASDF.TaggedMapping("!core/software-1.0.0", sw)))
# the file contains  !<!core/software-1.0.0>  because shorthand_tag only rewrites the tag:stsci.edu:asdf/ prefix
ASDF.load_file(f)
# ERROR: could not determine a constructor for the tag '!core/software-1.0.0' at line 9, column 2
ASDF.load_file(f; extensions = true)["sw"].tag
# "!core/software-1.0.0"                               loaded as an unrecognized tag, with a warning

ASDF.write_file(f, OrderedDict("sw" => ASDF.TaggedMapping("tag:stsci.edu:asdf/core/software-1.0.0", sw)))
# the file contains  !core/software-1.0.0
ASDF.load_file(f)["sw"].tag
# "tag:stsci.edu:asdf/core/software-1.0.0"             round-trips as the core tag

Only "tag:stsci.edu:asdf/core/software-1.0.0" round-trips as the core tag. The mechanism is pre-existing and every example in the diff uses the full-URI form.

Suggested fix. One sentence in the docstring: the tag field is the full tag URI; the writer emits the !core/ shorthand itself.


Not reported

  • Unasserted test warnings. The new tests load files with extensions = true and emit "unrecognized tag" warnings without @test_logs. This follows the repository's existing convention, so it was dropped.
  • Two candidate findings (an IdDict false-positive on equal isbits values, and NDArray being walked by map) were investigated and found not to be defects: only hooked values and containers are tracked, and NDArray is not an AbstractArray.

Attached version: code-review-46c4bde.md

I've verified each code example by hand and would be happy to continue to iterate on this with you if you agree with the general direction.

@cgarling

Copy link
Copy Markdown
Member Author

Yeah I'm not that great at this type of thing either. There's a lot to look at here so it'll take me a while to go through but I appreciate the effort!

P.S. The write-up mentions Julia v1.13, are you using 1.13 now? I haven't even tried it yet

@icweaver

Copy link
Copy Markdown
Member

Oh, yea, it already landed in juliaup and I wanted to give it a spin. The new syntax highlighting system is really nice!

@cgarling

Copy link
Copy Markdown
Member Author

I revised the implementation around the simpler one-argument  to_tree(value)  hook and removed WriteContext. Recursive traversal and cycle tracking are now private. Converter results are redispatched, tuples and named tuples are handled, unsupported leaves and keys fail explicitly rather than being stringified, and multidimensional arrays now fail clearly instead of being serialized as display text. The writer also performs conversion and float normalization in one pass. I simplified the tests and API documentation, switched the example to deterministic mappings, and documented full tag URIs.

A minor annoyance; compatible multidimensional numerical arrays nested in converter output are not automatically converted to NDArrayWrapper, callers must wrap them explicitly. We could write a method like to_tree(::AbstractArray) -> NDArrayWrapper or something to improve ergonomics, but I haven't added it here.

This branch has not been deployed

No deployments
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