Typed exceptions and logging hygiene (umbrella #46) - #52
Merged
Conversation
…d-logging # Conflicts: # pyproject.toml # uv.lock
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
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.
Closes #43, #44, #45, #47, #48, #49 — the whole of umbrella #46.
Two commits, matching the umbrella's two halves: the error contract, then logging hygiene.
1. Typed exceptions, and raising instead of failing silently
New
oshconnect/exceptions.py:OSHConnectErrorsubclassesException, so existingexcept Exception:handlers are unaffected. All fields are optional and callers should check forNone— not every call site has all of them.#47 — the five bare
raise Exceptionsites are converted. Rather than five near-identical blocks, the shared contract (POST → check ok → readLocation) now lives in one helper,new_resource_id_from_response()inresources/base.py, so all five report failures identically. Note this changes the message wording from "Failed to insert system" to "Failed to create system" for uniformity across resource types.#44 — that helper uses
.get('Location'), so a 2xx without the header raisesMissingLocationHeaderErrorexplaining the resource was probably created and the caller should re-discover rather than re-POST (avoiding a duplicate), instead of a bareKeyError: 'Location'pointing at a dict lookup.#43 —
add_system_to_node,create_and_insert_system, and_insert_systemshared anif target_node in self._nodes:guard with noelse, silently returningNone. All three now route through_require_registered_node()and raiseConfigurationError.#49 —
Node.discover_systems()raisesResourceDiscoveryErrorinstead of returningNone, so a 401 is no longer indistinguishable from an empty server (thefor s in discover_systems() or []:idiom turned auth failures into zero-iteration loops). An empty server still returns[].System.discover_datastreams/discover_controlstreamswere callingres.json()with nores.okcheck at all — a failed listing raised a JSON decode error; both now raise the typed error too.Bug found along the way
repr(Node)raisedAttributeError.Nodeis a@dataclasswhose generated__repr__reads every declared field, but_basic_auth,_mqtt_client, and_nats_clientwere only assigned when credentials / transports were enabled. So any anonymous or transport-less node — most of them — blew up when repr'd. It was already visible in pytest failure output (<[AttributeError(...) raised in repr()] Node object>) and surfaced here when a new error message tried to interpolate the node. All three fields are now bound up front, with a regression test.2. Logging hygiene
#48 — 24 log calls across 6 modules went to the root logger via
logging.warning(...). Consequences: an app couldn't raise OSHConnect to DEBUG or silence it without reconfiguring the root logger, and the first warning auto-installed a stderr handler on the consumer's behalf. All 24 now use a module-levellogger = logging.getLogger(__name__), and the package logger carries aNullHandler.tests/test_logging.pypins this with a source-scanning guard, so a newlogging.warning(...)anywhere undersrc/oshconnectfails CI rather than silently reintroducing the leak.Also corrected the
SchemaFetchWarningdocstring, which namedNode.discover_systemsas a source of schema-fetch failures — that method never fetches a schema.#45 —
System.insert_self()'s server-facing path had no CI coverage: the only test driving it lives in the@pytest.mark.networksync module, which CI skips.tests/test_node_sync_mocked.pyimports the real_ensure_dest_systemhelper (markers there are per-test, so it's importable) and drives it against mocked HTTP, covering reuse, create, and failure-propagation.Verification
492 tests pass (up from 484),
flake8 src/clean, strict Sphinx build succeeds. The newexceptionsmodule is wired intodocs/source/api.rst, and README gains a Logging section documenting theoshconnectnamespace.Version
0.5.3a1→0.5.5a1.0.5.4a0is deliberately skipped — it's claimed by #51, which is still open.Compatibility
except Exception:keeps working everywhere.Node.discover_systems()raises where it returnedNone; callers relying on the falsy return were treating failures as empty results, which is the bug.insert→create); anything matching on that exact string needs updating. Matching on the type is now the better option.