feat: type Controller's mqttc by what the SDK calls on an injected client - #12
Conversation
9a9fb96 to
47e33be
Compare
…ient Closes electrification-bus#8. mqttc was annotated with the concrete MqttClient, which nominally admits only that class or a subclass. It works today only because ebus-mqtt-client ships no py.typed, so downstream type checkers resolve the import to Any. When that marker lands the annotation starts meaning what it says, and a consumer injecting its own client needs a cast or a type-ignore to use a feature whose purpose is supplying one. Adds MqttTransport, a runtime_checkable Protocol of the three members the SDK actually calls on an injected client — publish, subscribe, unsubscribe — and widens the parameter to it. Signatures mirror MqttClient exactly, so MqttClient satisfies it unchanged and every existing call site keeps type-checking. Deliberately not the full client surface. An injected client is never started or stopped: _connect_broker returns early when mqttc is already set, so the start() beside from_config is unreachable, and stop() is behind if self._owns_client. is_connected, is_running and publish_and_flush are Device/Property-path members with no injection point. Typing the parameter with those would oblige consumers to implement methods the SDK provably never calls on their object. Two supporting edits, both behaviour-preserving: _connect_broker binds the SDK-built client to a local of the concrete type so start() resolves, and Controller keeps that client in _owned_client so stop() has a typed handle. Assignment order in _connect_broker is unchanged, so a start() that raises leaves self.mqttc exactly as it did before.
47e33be to
e16a817
Compare
dcj
left a comment
There was a problem hiding this comment.
Requesting one change before merge; full rationale is in discussion #7 (the transport-seam thread). Make MqttTransport the shared base and derive the role-specific contracts from it, rather than typing the consumer surface as MqttTransport directly:
MqttTransport(base):publish,subscribeMqttControllerTransport(MqttTransport): addsunsubscribe— this PR's injection pointMqttDeviceTransport(MqttTransport): addsis_connected,is_running— lands with the Device seam, not here
Two reasons. The general name goes to the general surface instead of the consumer-specific one. And because the future Device contract derives from the two-member base rather than the three-member consumer type, it never inherits unsubscribe, so the wart you flagged dissolves instead of being worn. MqttClient still satisfies all three structurally, and your docstring line about "a second protocol deriving from this one" becomes literally correct.
Everything else here is good as-is; the self.mqttc / self._owned_client split that makes "never started, never stopped" a property of the types is exactly the right move. Happy to merge as soon as the rename + base land.
…tract MqttTransport carries publish + subscribe, the surface both injection points share. MqttControllerTransport derives from it and adds unsubscribe, the only member the consumer path reaches beyond that base. A Device-side contract derives from the same base, so it picks up is_connected and is_running without inheriting unsubscribe, which nothing on that path calls.
#14) (#18) Lets a root Device accept a caller-supplied MQTT client so the SDK can be embedded in a host that owns its connection (Home Assistant: single_config_entry makes a second SDK-owned connection impossible). Mirrors Controller's seam. - Device(mqttc=): root-only, mutually exclusive with mqtt_cfg= and parent=. _owns_client = mqttc is None gates lifecycle. Typed Optional[MqttClient] for now (the MqttDeviceTransport protocol widening rides #12). - start_mqtt_client() (Device and Property) no-ops for an injected client. - stop() branches on ownership: injected publishes a plain retained $state=disconnected and returns without flushing or closing (non-blocking). - publish_value()/clear_value() gate on connectivity (is_running OR is_connected) so a loop-owning host that never calls start() still publishes values; owned behavior is unchanged (connected implies running there). - on_disconnect= is documented and warned as inert for an injected client. - Presence-by-identity for the mqtt_cfg exclusivity guard. Caller wires the Homie-correctness pieces from #13: set will() before connecting and call refresh_tree() from their on-connect. docs: README Bring-your-own-transport section; CHANGELOG [Unreleased] entries (also backfills #13's will()/resync()). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dcj
left a comment
There was a problem hiding this comment.
This is exactly the split from discussion #7: MqttTransport is now the shared base (publish + subscribe), MqttControllerTransport derives and adds unsubscribe, and the docstring correctly notes a Device-side contract derives from the same base with is_connected/is_running and never inherits unsubscribe. The general name lands on the general surface, and the wart is gone.
Verified locally (this branch predates the 3.10-3.13 matrix, so its CI is ruff-only): full suite passes and ruff check + ruff format --check are clean. Nice split of self.mqttc (the transport) from self._owned_client (the concrete client start/stop resolve on).
Approving. The Device-side MqttDeviceTransport(MqttTransport) follow-up (plus widening Device.mqttc) is mine, on top of this once it lands.
…lectrification-bus#8) Bill's electrification-bus#12 typed the bring-your-own-transport injection points with narrow Protocols; add the [Unreleased] entry (merged main in first so it sits with the other unreleased entries rather than conflicting). Co-Authored-By: Bill Flood <cayossarian@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dcj
left a comment
There was a problem hiding this comment.
Re-approving after syncing main in and adding the CHANGELOG entry for the transport Protocol typing. Full matrix (3.10-3.13) + ruff green. Merging on Bill's behalf.
Follow-up to #14, on top of #12's MqttTransport base. Adds a Device-side structural type and mirrors #12's owned/injected client split onto Device, so "the SDK never starts or stops a client it did not build" is enforced by the types rather than a convention. - transport.py: MqttDeviceTransport(MqttTransport) adds is_connected + is_running (the members the device publish path reads); omits start/stop/publish_and_flush, which are owned-only. Exported from ebus_sdk. - Device.mqttc is retyped Optional[MqttDeviceTransport]; the SDK-built client is kept on a separate Device._owned_client (concrete MqttClient), and every owned-only call (start/stop/publish_and_flush, incl. Property.start_mqtt_client) routes through it. get_mqtt_client() returns the transport type. +4 tests (MqttDeviceTransport protocol + the _owned_client handle). Full suite and ruff green. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… typing) Bumps __version__ to 0.16.0 and promotes the [Unreleased] CHANGELOG to [0.16.0]. Carries the bring-your-own-transport work accumulated since 0.15.0: the Device(mqttc=) producer seam (#14) with the will/refresh_tree wiring contract (#13), and the transport Protocol typing (MqttTransport / MqttControllerTransport #12, MqttDeviceTransport #19). README gains the BYO section with a neutral note on the shared-connection will limit. This is the ebus-sdk release the ebus-mqtt-client py.typed marker (electrification-bus/ebus-mqtt-client#9) is gated on. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… typing) (#21) Bumps __version__ to 0.16.0 and promotes the [Unreleased] CHANGELOG to [0.16.0]. Carries the bring-your-own-transport work accumulated since 0.15.0: the Device(mqttc=) producer seam (#14) with the will/refresh_tree wiring contract (#13), and the transport Protocol typing (MqttTransport / MqttControllerTransport #12, MqttDeviceTransport #19). README gains the BYO section with a neutral note on the shared-connection will limit. This is the ebus-sdk release the ebus-mqtt-client py.typed marker (electrification-bus/ebus-mqtt-client#9) is gated on. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Closes #8. Two module-level choices are still open at the end, each with a recommendation,
so agreeing is a merge rather than a task.
Updated after review — the rename is in (
266e329)Your split is better than what I proposed, and for a reason I had missed: putting the
general name on the general surface means a
Devicecontract derives from the base, sounsubscribeis never inherited. The wart doesn't get worn, it stops existing. The docstringline "a second protocol deriving from this one" is now literally true rather than
aspirational.
MqttTransport—publish,subscribe. The surface both injection points share.MqttControllerTransport(MqttTransport)— addsunsubscribe. What this PR wires.runtime_checkable, both exported;MqttClientsatisfies each unchanged.A
Device-side contract is deliberately absent — it lands with the Device seam.test_the_controller_contract_derives_from_the_shared_basepins the property that makes thesplit worth doing: a two-member client satisfies
MqttTransportand is rejected byMqttControllerTransport.The failure, reproduced
ebus-mqtt-clientships nopy.typed, soMqttClientcurrently resolves toAnydownstream and
Optional[MqttClient]accepts anything. That is the only reason injectiontype-checks today. Dropping a
py.typedinto the installedebus-mqtt-clientsimulates therelease #8 anticipates:
Re-run after the rename, so it is evidence about the merged shape rather than the first one.
Where the three surfaces come from
I mapped every MQTT-client call site in
homie.pyandha/bridge.pyto its enclosing classbefore choosing. The same table derives your split:
publishandsubscribeappear on bothpaths,
unsubscribeonly on theControllerpath,is_connected/is_runningonly on theDevicepath,start/stoponly when the SDK owns the client.Line numbers below are
main@bd060d9(0.15.0), not this branch — evidence aboutexisting code. On the branch head they shift by +2 for Property/Device sites (the added
import) and +13 for Controller sites (that import, plus the
_owned_clientblock and the_connect_brokerrewrite); re-verified after the rename. Thestart/stopsnippets beloware the two lines this diff rewrites.
Controllerpath (injectable)Device/Node/Propertypathpublish:2742,:2811,bridge.py:295:787,:834,:1656,:1664,:1730,:1869subscribe:2392–:2535,bridge.py:176:957unsubscribe:2609,bridge.py:211is_connected:1473,:1501is_running:690,:744,:821,:1455publish_and_flush:1504start:2311:691,:1456stop:2914:1510Under the flush change you described on #7 —
_owns_clientdeciding both "flush?" and"stop?" — that
publish_and_flushrow becomes owned-only too, which is what leaves aDevicecontract atpublish,subscribe,is_connected,is_running.startandstopare unreachable for an injected client today, and that is enforced ratherthan conventional:
0.15.0 corroborates this from your side.
Controller.__init__:2281-2284on the disconnecthook: "Only effective when the controller OWNS its client (constructed from
mqtt_cfg); abring-your-own-client caller registers disconnect handling on its own client." The hook is
passed to
MqttClient.from_config(...)at:2309, inside the owned branch — so it adds nomember to the injected-client surface.
Typing the parameter with the full client surface would therefore demand five methods the SDK
provably never calls there — including two whose non-invocation is the documented guarantee
of the feature. A consumer supplying a connection whose lifecycle it manages elsewhere would
have to stub them as pure ceremony.
Signatures mirror
MqttClientexactly,Anyonsubscribe's callback included. Returns areobjectbecause every call site discards them.Correcting something I said in #8
#8 claimed "annotation only, nothing existing has to be edited." That was wrong, and the
gap is small but real: with
self.mqttctyped as the protocol,start()andstop()nolonger resolve on it. Two behaviour-preserving edits:
_connect_brokerbinds the SDK-built client to a local of the concrete type, sostart()resolves. Assignment order is unchanged — both references are set before
start(), soa
start()that raises leavesself.mqttcexactly as it did before.Controllerkeeps that client in_owned_client, givingstop()a typed handle._owns_clientstill decides;_owned_clientisNoneprecisely when a client wasinjected, which makes "never stopped" a property of the types rather than a promise in a
comment.
Tests
Six in
TestMqttTransportProtocols:MqttClientsatisfies both protocols; both are exportedfrom the package root (a consumer who cannot name the type gains nothing); the derivation
holds and a two-member client is rejected by the
Controllercontract; a deliberatelythree-member client is accepted and driven through
start_discovery()andstop()withouteither being called on it; and
_owned_clientisNonewhen injected, set and cleared whenSDK-built.
pytest: 506 passed, on Python 3.14 and on 3.10 (the declared floor — your CI runs only3.12, so I checked the boundary explicitly).
ruff check .andruff format --check .cleanon the pinned 0.15.21.
Relationship to the transport-seam discussion
Discussion #7 argues the
same principle from the producer side. This PR is the
Controllerhalf and does not dependon it — every fact above comes from
_owns_client, the early return in_connect_broker,and the guarded
stop(), all of which exist onmaintoday. Noting it only so the narrownessreads as deliberate rather than as an oversight.
Two things you might want to decide, with a recommendation for each
Module placement — recommend keeping
transport.py. Onlyhomie.pyimports it today, sohomie.pywould work. I'd still keep it separate, for one reason beyond the 2,906-line file:transport.pyimports nothing buttyping, so it is a leaf any module can import withoutcycle risk. Folding it into
homie.pymeans anything wanting to name a transport type hasto import all of
homie.py—ha/bridge.pyalready imports from..homie(:49) andcurrently duck-types its client via
getattr/hasattr(:174,:209-210); a leaf module iswhat would let it annotate instead. Cheap to move if you disagree — one file, no call sites.
_owned_clientvs a narrower fix — recommend keeping it. Acastor anisinstanceassert at the
stop()site would be a smaller diff. I'd keep the attribute:assertisstripped under
-O, so an assert there would buy the type checker a guarantee the runtimedoes not actually make, and
castchecks nothing at any time._owned_clientmakes "aninjected client is never stopped" true by construction — it is
Noneprecisely when one wasinjected — for the cost of one
Optional[MqttClient]field. Say the word if you'd rather havethe three-line version.