You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#218 fixed required arguments at the top level of a tool's schema, and added a
dispatch-level check that enforces them before any handler runs. That check is
deliberately shallow: it validates presence of the names in the tool's own required list and does not descend into array items.
Three tools declare required fields inside an array's items schema and then
default every one of them.
The one that writes
batch_add_wire — items require ["x1","y1","x2","y2"] (sch_wiring.rs:45),
and the loop reads each with .as_f64().unwrap_or(0.0). A wire element missing
its coordinates becomes a wire from (0,0) to (0,0), which is then snapped to
grid, added to the schematic, counted in added_wires, and run through
T-junction detection. {"wires": [{}]} adds a degenerate wire and reports
success.
The two that build library content
create_footprint — pad items require ["number","type","shape","x","y","width","height"] (library.rs:58) and
default all seven (library.rs:685-692): "1", "smd", "rect", 0.0, 0.0, 1.0, 1.0. A {} pad becomes a 1x1 mm SMD rect pad numbered 1 at
the origin. Since Eight tools declare arguments required, then silently default them #218's fix pads itself can no longer be omitted, but its contents are still unchecked — and a footprint whose pads all landed on the
origin is worse than one with no pads, because it looks plausible.
create_symbol — unit items require ["pins"] (library.rs:182), defaulted
to an empty vec, producing a pin-less unit.
Ten other tools with nested required enforce it correctly — via let Some(..) else, an explicit match … None => error, read_xy_pair, parse_points, or serde_json::from_value failing on a Null. So the
convention exists; these three predate or missed it.
The decision this needs
Unlike the top-level case there is no single obvious behaviour, which is why
this is not folded into #218:
Refuse the whole call when any element is malformed — consistent with how
a missing top-level argument now behaves, and keeps a batch atomic.
Skip the element and report it — matches how several batch tools already
handle a per-element failure (errors: [...] alongside a count), and lets a
20-wire batch with one bad entry still do 19 useful things.
I lean to refusing for create_footprint/create_symbol, where the output is
a single artefact that is either right or not, and to skip-and-report for batch_add_wire, where the batch shape already implies partial progress and
the tool already returns an errors array. But that is a judgement, not an
obvious answer.
Whichever is chosen, the error should name the index as well as the field —
"each point needs a numeric 'x'" tells you nothing about a twelve-element
batch. parse_points (pcb_components.rs:113) is the existing pattern for
that.
#218 fixed required arguments at the top level of a tool's schema, and added a
dispatch-level check that enforces them before any handler runs. That check is
deliberately shallow: it validates presence of the names in the tool's own
requiredlist and does not descend into array items.Three tools declare
requiredfields inside an array'sitemsschema and thendefault every one of them.
The one that writes
batch_add_wire— items require["x1","y1","x2","y2"](sch_wiring.rs:45),and the loop reads each with
.as_f64().unwrap_or(0.0). A wire element missingits coordinates becomes a wire from (0,0) to (0,0), which is then snapped to
grid, added to the schematic, counted in
added_wires, and run throughT-junction detection.
{"wires": [{}]}adds a degenerate wire and reportssuccess.
The two that build library content
create_footprint— pad items require["number","type","shape","x","y","width","height"](library.rs:58) anddefault all seven (
library.rs:685-692):"1","smd","rect",0.0,0.0,1.0,1.0. A{}pad becomes a 1x1 mm SMD rect pad numbered 1 atthe origin. Since Eight tools declare arguments required, then silently default them #218's fix
padsitself can no longer be omitted, but itscontents are still unchecked — and a footprint whose pads all landed on the
origin is worse than one with no pads, because it looks plausible.
create_symbol— unit items require["pins"](library.rs:182), defaultedto an empty vec, producing a pin-less unit.
Ten other tools with nested
requiredenforce it correctly — vialet Some(..) else, an explicitmatch … None => error,read_xy_pair,parse_points, orserde_json::from_valuefailing on aNull. So theconvention exists; these three predate or missed it.
The decision this needs
Unlike the top-level case there is no single obvious behaviour, which is why
this is not folded into #218:
a missing top-level argument now behaves, and keeps a batch atomic.
handle a per-element failure (
errors: [...]alongside a count), and lets a20-wire batch with one bad entry still do 19 useful things.
I lean to refusing for
create_footprint/create_symbol, where the output isa single artefact that is either right or not, and to skip-and-report for
batch_add_wire, where the batch shape already implies partial progress andthe tool already returns an
errorsarray. But that is a judgement, not anobvious answer.
Whichever is chosen, the error should name the index as well as the field —
"each point needs a numeric 'x'" tells you nothing about a twelve-element
batch.
parse_points(pcb_components.rs:113) is the existing pattern forthat.