spytial-core 3.2.1 + inferredEdge draw; document the decorators; align hold/projection/value-sets#128
Conversation
Every decorator is built by _create_decorator and takes **kwargs, so help() and IDE hovers showed `decorator(**kwargs)` and nothing else. The reference docs lived only on the annotation *classes* (the Annotated[...] form), which is not where you look when writing `@spytial.group(`. That made a complete feature read as missing: group's addEdge has supported the three directions and the GroupEdge style block since #124/#127, but you could not see it. _create_decorator now takes doc= and sets __name__/__qualname__/__doc__, and all 17 decorators document exactly the keys spytial-core reads -- verified by parsing the emitted YAML with core's parseLayoutSpec rather than trusting the existing prose. That verification turned up three divergences from core: - hold='never' is core's only negation mechanism (parseConstraints reads it off the inner block), but CONSTRAINT_TYPES omitted it, so every use printed "Unknown fields" while quietly working. Now accepted on all four constraints, and documented in docs/operations.md, which never covered it. - projection is a no-op. Core's layout-spec parser does not read it at all -- projection is a pre-layout transform over the data instance, driven by the viewer's controls, and core's projection-transform module is explicit that it is "intentionally decoupled from the layout engine". Authoring one emitted YAML core discards. Now warns on all four authoring paths (exactly once each); still serializes, since this is a deprecation, not a removal. - group's field form listed showLabel, which core never reads: it builds GroupByField(field, groupOn, addToGroup, selector, negated) and derives label visibility from negation alone. Also fixes two class docstrings whose examples core rejects: Align advertised direction='left' (align takes only horizontal/vertical -- core raises), and Orientation advertised directions=['horizontal'] (matches no case in core's direction switch, so it is silently dropped). The two vocabularies had been swapped. Where spytial-py is deliberately stricter than core -- @ICON requires showLabels, @Cyclic requires direction, @group requires name even when negated -- the docstrings say so rather than promise core's defaults. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06a858e647
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "cyclic": {"required": ["selector", "direction"], "optional": ["hold"]}, | ||
| "orientation": {"required": ["selector", "directions"], "optional": ["hold"]}, | ||
| "align": {"required": ["selector", "direction"], "optional": ["hold"]}, |
There was a problem hiding this comment.
When the decorator/object/type-alias paths now accept hold, they only pass through validate_fields, so values are not checked or normalized like the Orientation/Cyclic/Align/Group constructors do. In those paths, @spytial.orientation(..., hold='neevr') or hold='always' is stored verbatim in the emitted YAML; since the new comment says core only flips the constraint for hold: never, a typo silently leaves the constraint positive/default instead of negated. Please validate the new kwarg on these paths and omit the default always before appending entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — confirmed and fixed in f67a9e1.
You're right that adding hold to CONSTRAINT_TYPES only taught validate_fields the key was allowed; it never looks at values. Reproduced both halves, and confirmed the consequence against core's parser:
hold: never -> negated = true
hold: neevr -> negated = false # silently positive
hold: always -> negated = false
So @spytial.orientation(..., hold='neevr') emitted a constraint that stayed positive — the inverse of what was written, with no error. That's the same silent-divergence class of bug this PR set out to close, so it's squarely in scope rather than a nitpick.
The Annotated[...] classes already had the right contract (reject unknown values, omit the no-op always), so rather than restate it I extracted _validate_hold/_normalize_hold, called from the decorator and _annotate_object — the two **kwargs choke points, covering @decorator on classes and objects plus annotate(). The four class constructors now share the helper instead of carrying four copies of the same check. Normalization is idempotent, since the decorator-on-object path passes through both.
On dropping always: agreed, and worth noting it was a real inconsistency, not just noise — the class form omitted it while the decorator emitted it, so the same spec differed depending on which form authored it. Both omit now.
26 new tests cover it across all four constraints and both paths; 399 pass.
Codex caught that adding `hold` to CONSTRAINT_TYPES only taught validate_fields the key was allowed -- it never checks values. So the decorator and object paths took `hold` as an opaque string and passed it straight into the YAML, where core reads anything that isn't exactly 'never' as "not negated". `@spytial.orientation(..., hold='neevr')` therefore emitted a constraint that silently stayed positive: the inverse of what was written, with no error and nothing to see. Verified against core: hold=never -> negated=true, hold=neevr -> false. The Annotated[...] classes already had the right contract (reject unknown values, omit the no-op 'always'), so the fix is to share it rather than restate it: _validate_hold/_normalize_hold, called from the decorator and _annotate_object, with the four class constructors folded onto the same helper instead of four copies of the same check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Generalizes the hold fix: hold was one instance of a closed value set that
nothing checked. Core's RelativeDirection / RotationDirection /
AlignDirection are TypeScript unions, so they are erased at runtime and no
downstream code re-checks them. Verified what core actually does with a
value outside each set:
orientation.directions ['bogus'] -> kept, matches no case, constraint
silently evaporates
cyclic.direction 'widdershins' -> kept, read as 'clockwise', so a
typo'd 'counterclockwise' silently
spins the other way
flag.name typo -> silent no-op
align.direction 'sideways' -> core throws (the only checked one)
Three of four fail silently, so the authoring site is the only place they
can surface. ORIENTATION_DIRECTIONS / ROTATION_DIRECTIONS /
ALIGN_DIRECTIONS / FLAG_NAMES now mirror core's unions and are enforced on
every authoring form -- including the Annotated[...] classes, via one hook
in SpytialAnnotation, so the two forms cannot disagree.
Two things fell out of doing it:
- There were three **kwargs paths, not two. annotate_type_alias open-coded
the desugar/coerce sequence and so never got the hold fix or the
projection warning -- the drift Codex pointed at, one path over. All
three now share _prepare_kwargs.
- suggest's _ORIENT_DIRS restated the vocabulary and omitted directlyAbove
and directlyBelow, so a model proposing either had a direction core
handles filtered out from under it. It now sources the canonical set.
The check immediately found 25 call sites across 6 test files, a demo,
pre_publish_check and the copilot instructions using align's axis words as
orientation directions -- i.e. every one of them was asserting on a
constraint that does nothing. That is the Orientation docstring bug from
the previous commit, propagated by copy. Fixed, along with Flag(name=
'debug') and Align(direction='left'), which were the same thing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3.2.0-beta.1 was never published — npm went straight to 3.2.1, now `latest`.
Pin, vendored evaluator and version docstring all move via
update-spytial-core.sh. (The vendored evaluator.js is byte-identical
3.1.1 -> 3.2.1: draw is a layout/render concern, not an evaluator one.
Confirmed by diffing a fresh `npm pack` of both, so the empty diff is real
and not a copy that silently no-op'd.)
Adds `draw: '<end> -> <end>'` to inferredEdge — each end `_` or a group
constraint name, a named end attaching to the hull of the group keyed by
that end's atom. This is the first way to express group-to-group and
node-to-group edges. The shape is validated at the authoring site (core
rejects a malformed draw too, but only once the spec reaches the browser);
whether a named group *exists* needs the whole spec and may be declared on
another class, so that check stays core's.
Verified end to end rather than by round-tripping YAML alone: rendered a
real diagram against the published 3.2.1 bundle and read the geometry back
out of the DOM. The edge is crimson / 6,4 dashed / 3px — the lineStyle as
authored in Python — and both endpoints sit on group-hull perimeters.
That render caught a bad example this commit would otherwise have shipped.
`draw` resolves an end by looking up the group of that name *keyed by* that
end's atom, so the group must come from a binary selector. The example used
`{ t : Team, m : Member | m in t.members }`, which forms no groups at all
for a List[Member] field (members relationalizes as Team -> list -> Member,
so `m in t.members` never holds); and the docs' usual `Team.members` idiom
is unary, giving one unkeyed group, against which draw is dropped with no
edge, no warning and no error. Examples now use a keyed binary selector,
and both the docs and the docstring say the key is the requirement.
Version: minor, matching the convention for a core bump that brings new
capability (#127 core 3.1.0 -> 1.7.0; #105 core 2.9.0 -> 1.5.0).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
sidprasad
changed the title
1 participant
Started from "
@groupdoesn't seem to have the addEdge/direction stuff I need — or at least it's not well documented." The feature was entirely there and correct; it was invisible. Pulling that thread found a family of related problems, all the same shape: a value that looks applied but isn't.Ships spytial-py 1.8.0 on spytial-core 3.2.1.
spytial-core 3.1.1 → 3.2.1, and
draw3.2.0-beta.1 was never published — npm went straight to 3.2.1. Pin, vendored evaluator and version docstring move via
update-spytial-core.sh. The vendoredevaluator.jsis byte-identical across the bump (drawis a layout concern, not an evaluator one); confirmed by diffing a freshnpm packof both, so the empty diff is real rather than a copy that silently no-op'd.New:
draw: '<end> -> <end>'oninferredEdge. Each end is_or a group-constraint name; a named end attaches to the hull of the group keyed by that end's atom. It's the first way to express group-to-group and node-to-group edges. The shape is validated at the authoring site (core rejects a malformeddrawtoo, but only once the spec reaches the browser); whether a named group exists needs the whole spec and may be declared on another class, so that stays core's check.Verified by rendering a real diagram against the published 3.2.1 bundle and reading the geometry back out of the DOM — not by round-tripping YAML alone:
That render caught a bad example this PR would otherwise have shipped — see below.
The original problem
Every decorator is built by
_create_decoratorand takes**kwargs, sohelp()and IDE hover showeddecorator(**kwargs)and nothing else. The reference material lived on the annotation classes (theAnnotated[...]form) — not where you look when typing@spytial.group(.group'saddEdgehas supportednone/togroup/fromgroupplus theGroupEdgeblock since #124/#127, with no way to discover it from the call site._create_decoratornow takesdoc=and sets__name__/__qualname__/__doc__. All 17 decorators document exactly the keys core reads — verified against core's realparseLayoutSpec, not the existing prose.What verification found
Closed value sets were unchecked, and core fails silently on three of four. The direction unions are TypeScript types — erased at runtime, so nothing re-checks them:
cyclic.direction'widdershins'counterclockwisesilently spins the other wayorientation.directions['bogus']flag.namealign.direction'sideways'Now enforced on every authoring form, including the
Annotated[...]classes via one hook inSpytialAnnotation, so the two forms can't disagree.The check found 25 call sites that were already wrong — 6 test files, a demo,
pre_publish_check.py, the copilot instructions — all using align's axis words as orientation directions, i.e. asserting on constraints that do nothing. They trace to a wrong example in theOrientationdocstring, propagated by copy.Flag(name='debug')andAlign(direction='left')were the same. That's the argument for the check: the docs taught the wrong vocabulary and nothing downstream objected.hold='never'was broken. Core's only negation mechanism, butCONSTRAINT_TYPESdidn't list it — every use printedWarning: Unknown fieldswhile quietly working. Now accepted, validated, with the no-opalwaysdropped to match the class form.There were three
**kwargspaths, not two.annotate_type_aliasopen-coded the desugar sequence and so never got theholdfix or the projection warning. All three now share_prepare_kwargs.suggestrestated the vocabulary and got it wrong._ORIENT_DIRSomitteddirectlyAbove/directlyBelow, so a model proposing either had a direction core handles filtered out from under it. It now sources the canonical set.projectionis a no-op — deprecated. Core's parser never reads it:projection: {sig: Node}yields zero directives and no trace. It's a pre-layout transform driven by the viewer's controls; core's own module says it is "intentionally decoupled from the layout engine." Now warns on all four authoring paths, exactly once each. Still serializes: deprecation, not removal.group's field form listedshowLabel, which core never reads.A
drawexample that didn't work.drawneeds a group from a binary selector, since it resolves each end by the group keyed by that atom. My example{ t : Team, m : Member | m in t.members }forms no groups for aList[Member]field (membersrelationalizes asTeam -> list -> Member, som in t.membersnever holds), and the docs' usualTeam.membersidiom is unary — one unkeyed group, against whichdrawis dropped with no edge, no warning, no error. Only rendering it surfaced this. Examples now use a keyed binary selector and both docs and docstring state the requirement.Left strict, documented rather than changed
@iconrequiresshowLabels(core reads missing asFalse; theIconclass defaults it toTrue— looks like an oversight),@cyclicrequiresdirection(core defaultsclockwise),@grouprequiresnameeven when negated (core synthesizes one). Happy to loosen any separately.Testing
443 pass, up from 323. New
test/test_decorator_docs.pycovers the decorator surface, the value sets across all four authoring paths (including that the check isn't narrower than core),hold, projection, anddraw. Every@spytial.*example indocs/is executed against the validator.One judgment call: out-of-vocab values raise rather than warn. It's a behavior change for anyone passing one today — but they're currently getting a constraint that does nothing, or the opposite direction.
🤖 Generated with Claude Code