some updates for dev documentation - #10065
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 5 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Updates only development documentation with structured guidelines for component design, configuration validation, testing doubles, and error handling. No behavioral, operational, or config changes.
Re-trigger cubic
ogenstad
left a comment
ogenstad
left a comment
There was a problem hiding this comment.
Looks good to me overall, but added two comments.
|
|
||
| Anything that comes from outside the component's own domain — loaded settings, an external service client, a telemetry sink — is resolved at that entry point, never inside the component: | ||
|
|
||
| - **Settings resolve in the factory, not the component.** A component takes plain values (`window_seconds: float`, `max_retries: int`), never a `Settings` object and never a module-global read. The factory is then the only place that knows a value came from configuration, which is also what makes the component directly testable with hand-picked values. |
There was a problem hiding this comment.
Do we have any examples of this? In situations where there's a high number of settings i thing a Settings object can be cleaner. An example would be the Config object for the SDK.
There was a problem hiding this comment.
I think the Config is a special case b/c it is a settings object. its only responsibility should be tracking many settings. any components that actually do things should have settings injected when they are constructed.
| for observer in self._observers: | ||
| try: | ||
| observer.on_depth_changed(queued=queued, running=running) | ||
| except Exception: |
There was a problem hiding this comment.
I don't think we should include except Exception as a good example. (https://docs.astral.sh/ruff/rules/blind-except/)
Instead we should have a specific exception even if it's made up so that the LLM doesn't pick up on this part and start to introduce additional broad exceptions. We also have #10002.
There was a problem hiding this comment.
I think in the case of an "observer" object (or any other "best-effort side-effect" components that should not block the primary logic during failure) we want to catch the bare Exception. we could move the bare Exception catch into on_depth_changed in this example and re-raise a different error class, but the end result would be the same
|
|
||
| - **Use a `Protocol`, not an ABC, and declare it beside the component that depends on it.** Structural typing is what makes this work: the adapter satisfies the protocol by having matching signatures, so it never imports the protocol's module and the dependency does not run backwards. An ABC does not have that property — the adapter must subclass it, and therefore import whatever module it lives in. If you genuinely need an ABC, it goes in a module of its own that both sides import, never in the consumer's module. | ||
| - **Name the methods in the depending component's vocabulary**, not the adapter's — `on_depth_changed(*, queued: int, running: int)`, not `set_gauges(...)` — and pass the values as arguments rather than handing over `self`, so the adapter can never read back into the component. The component then depends on a shape it defined, has no idea what is on the other side, and stays free to change its internals. | ||
| - **Put the concrete adapter in a separate, purpose-named module** that is the only place importing the library, and **let only the wiring layer import both** (see "Build components near the application entry point"). |
There was a problem hiding this comment.
Is there anything worth saying about explicitly declaring the protocol in the class definition?
I mean this:
class MyClass(MyProtocol):
# Protocol implemented
And not:
class MyClass():
# Protocol implemented implicitly
The upside of the first option is the direct link between the implementation and the protocol it satisfies. The downside is that we would need to import the protocol. Another option would be to make sure these protocols live in a package that is safe to import from anywhere.
There was a problem hiding this comment.
I'm always ready to say more about dependency injection. added some more detail to describe either using a Protocol without importing it or using a Protocol/ABC interface defined in a separate module
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. The PR adds only documentation (backend design, testing, configuration guidelines). No behavior, configuration, API, or operational change exists. The diff is bounded and clearly beneficial.
Re-trigger cubic
3 participants
trying to distill the changes from #10052 into something more repeatable
truly not sure if some of this might be better as
dev/guides/backend/creating-new-components.mdSummary by cubic
Adds clearer backend dev docs on interfaces/protocols, configuration validation, and testing doubles. Uses the API backpressure work to show how to wire observers, contain failures, and keep third‑party deps out of core logic.
Protocolnext to the consumer or an explicit interface module; never put an ABC in the consumer; name methods in the consumer’s vocabulary; keep adapters in their own module; only the wiring layer imports both; verify no third‑party packages appear under the logic by grepping the import graph.ge/le,allow_inf_nan=False); enforce cross‑field rules with@model_validatorand name the rule; test the validator and shipped defaults; use enums for values that leave the process (e.g., metric labels) and pass enum members, not strings._notifyfans out with per‑observer failure containment; wiring inserver.pychooses sinks soapi/admissionlogic stays decoupled behindProtocols.Written for commit 380efeb. Summary will update on new commits.