Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #402 +/- ##
=======================================
Coverage 91.25% 91.25%
=======================================
Files 72 72
Lines 2892 2892
=======================================
Hits 2639 2639
Misses 253 253 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
coretl
commented
Jul 21, 2026
This was referenced Jul 21, 2026
Incorporate coretl's inline review decisions: drop DataType (python types + *Meta), unify callback IO into the @attr factory, the __init__ filler rule + no-hints/Optional/external-add, nested Limits with inheritance, get_setpoint + ControllerRunner(start/stop) owning reconnect, severity enum, args/returns typed separately (kwargs -> spike #403), enum/Table handling, and dropping the Device.disconnect proposal for connect(force_reconnect=True) + atexit. Remaining deferred items (@shihab-dls, @Tom-Willemsen) kept as explicit open questions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Define the *Meta TypedDicts + Unpack overloads for the procedural spelling, the superset Meta for generic extras (SCPIParam), attribute-stored resolved meta, and the filler's runtime validation of annotated metadata against the datatype. Clarify in ADR 13 that hinted attributes exist after __init__ (filler creates them unfilled), not after initialise(). Module home for the new names deferred to #406 (top-level API namespace: flat vs nested). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Per @Tom-Willemsen on PR #402 (r3621453680): SECoP devices are discovered entirely from an over-the-wire `describe`, so you never statically know something is a command without knowing its signature. There is no "known command, unknown args" middle case — P/T are either completely known (static `Command[P, T]`) or the whole structure is unknown (built at runtime). Remove the `Any` args/returns option and the `Command[Any, Any]` consequence; resolve the open question awaiting @Tom-Willemsen. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Fold the top-level examples/ scaffold into the fastcs.demo package (mirrors ophyd-async; examples install with fastcs[demo] and become the single source of the tutorial code). Replace the stale 3-style scaffold README with the final 5-example hello-world -> complicated-device ladder (two backends: temperature sim for steps 1-4, cut-down Eiger REST sim for step 5), keyed to issues #398/#404/#390/#405/#391. ADR 0014: spell out the "one spec object per declaratively-filled attribute" design - SCPIParam carries the binding token AND all metadata via Unpack[Meta], is the exclusive spec source (filler does not merge a separate *Meta extra), and pays for its ergonomics with runtime validation. Record why it is SCPIParam not SCPIMeta (a binding extra you instantiate, sibling of PvSuffix/TangoPolling, not a Meta TypedDict) and that it lives in the demo/ protocol layer, not core (decision 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Fold in the 2026-07-22 review decisions: ADR 0014: io= objects (ReadIO/WriteIO/ReadWriteIO) superseded by getter/setter callables on AttrR/AttrW/AttrRW. getter() -> T | Update[T]; setter -> None | T | Update[T] (value return = accepted/clamped readback, the sanctioned secop setpoint echo). Update[T] = value/timestamp/severity. Datatype optional when getter/setter given (inferred, unwrapping Update[T]). update_period: ONCE (default) / float / None (on-demand); no getter = @scan-fed soft. Access mode from which params exist; ReadIO trio dropped. Both prior open questions closed. ADR 0018: @attr is decorator-only (@attr / @attr(precision=3) + @x.setter); no @attr_r/@attr_rw, no free-function attr() factory; procedural is AttrR/AttrRW. Title + stale refs swept; 0013 refs swept too. demo README: five modules, four tutorials (the reusable-io= rung is gone); controllers.py repurposed to the composition/@scan/@command example folded into the declarative tutorial. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
…od, set()) Rename/split the get()/update(value)/put(value) trio so access mode and device-IO are legible from the member set: - .value -> .readback + .setpoint (read-only properties; mirror bluesky/ ophyd Location(setpoint, readback); presence tracks access mode). - no-arg update() -> poll() (returns the value); update_period -> poll_period (schedule only). Deletes set_update_callback/bind_update_callback. - update(value) is now a pure cache push (no IO, no None sentinel). - put() -> set() (bluesky verb); caches .setpoint then runs setter; setter's T|Update[T] return feeds .readback. sync_setpoint kwarg gone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Resolves the two review threads Shihab answered on 2026-07-22: - ADR 0019 (@scan): confirmed @scan is a purely-internal periodic coroutine bound to no Attr and surfacing no Signal; @command, by contrast, creates an AttrW and IS exposed. Closes the last open question on 0019; folded into the mapping table + Resolved-in-review. - ADR 0014 (setpoint echo): set() caching .setpoint is an attribute-cache guarantee only. Records Shihab's CA-vs-PVA divergence — PVA posts the setpoint immediately (may later alarm), CA posts only after the update callback completes, so a long-running setter delays the CA-visible setpoint. CA/PVA ordering realignment noted as a transport follow-up, not gating this rework. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
Fold the #402 review updates, "Resolved in review" statement dumps, and "Open questions" sections into one clean shape per ADR (Context / Decision / Consequences / Questions resolved in review). Make all seven consistent with the deleted io= and DataType: code examples are now all getter/setter + *Meta, and the runtime surface (.readback/.setpoint, poll()/poll_period, set()) is used uniformly across 0014/0016/0018/0019 instead of the old get()/put()/update_period/Waveform names. Preserves the @shihab-dls #402 replies (CA-vs-PVA setpoint visibility in 0014; @scan-vs-@command exposure in 0019), rewoven into the clean structure. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LgnovZ7FWY8YptwqiufTbX
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is not to be merged, but is there so we can make drive-by comments on lines of code as we see them be accepted in PRs into the refactor branch. When we are happy we will flip this PR to non-draft and this will become what is merged.