Skip to content

config: ConfigurationBuilder duplicates the dataclass constructor, and Configuration's retry keys have no consumer #67

@OmarAlJarrah

Description

@OmarAlJarrah

Two small but related shape problems in config/configuration.py that I think are worth tidying before the public surface freezes.

Current design

Configuration is a @dataclass(frozen=True, slots=True) that takes overrides and env directly, with a defensive copy of overrides in __post_init__. Lookup is layered (override -> env -> default) and the typed accessors (get_int / get_bool / get_duration) are clean and dependency-free.

Alongside it, the same module ships ConfigurationBuilder (packages/dexpace-sdk-core/src/dexpace/sdk/core/config/configuration.py, lines 241-291), re-exported in __all__ and from config/__init__.py, plus a Configuration.builder() classmethod. The builder's entire job is to accumulate an overrides dict and an env source, then call Configuration(overrides=dict(self._overrides), env=self._env) in build(). So Configuration.builder().put("A", "1").put("B", "2").build() is exactly Configuration(overrides={"A": "1", "B": "2"}) — same immutability, more ceremony.

Separately, the class defines five well-known keys as ClassVars (lines 72-76):

MAX_RETRY_ATTEMPTS: ClassVar[str] = "MAX_RETRY_ATTEMPTS"
REQUEST_RETRY_DEFAULT_TIMEOUT: ClassVar[str] = "REQUEST_RETRY_DEFAULT_TIMEOUT"
HTTPS_PROXY: ClassVar[str] = "HTTPS_PROXY"
HTTP_PROXY: ClassVar[str] = "HTTP_PROXY"
NO_PROXY: ClassVar[str] = "NO_PROXY"

The three proxy keys are genuinely consumed — util/proxy.py reads Configuration.HTTPS_PROXY / HTTP_PROXY / NO_PROXY in ProxySettings.from_configuration. The two retry keys are not. RetryPolicy reads its knobs from ctx.options in _configure_settings (pipeline/policies/retry.py, lines 272-285): options.get("retry_total", ...), options.get("timeout", ...), etc. Nothing in core reads MAX_RETRY_ATTEMPTS or REQUEST_RETRY_DEFAULT_TIMEOUT, and the per-call option names don't even match those keys, so there is no implicit bridge either.

Trade-off / concern

  1. The builder is the redundant noise the conventions explicitly call out. CLAUDE.md: "Builders are a Java idiom — Python's keyword and default arguments make them redundant noise." StagedPipelineBuilder earns its keep because assembling ordered, staged policies is awkward as a single constructor call. ConfigurationBuilder has no such excuse — the dataclass constructor already expresses everything it does, and the codebase already standardises a fluent-but-immutable idiom (with_* returning a new frozen instance) on Response, Url, Headers, Request, and the multipart bodies. The builder is the one place the Java port leaks through.

  2. The retry keys are dead advertising. A consumer who sets Configuration(overrides={Configuration.MAX_RETRY_ATTEMPTS: "5"}) and expects the default pipeline to honour it gets a silent no-op, because the retry policy never consults Configuration. Defining a well-known key that nothing reads is worse than not defining it — it implies a contract that does not exist.

Proposed direction

  1. Drop ConfigurationBuilder (and Configuration.builder()). If a fluent feel is still wanted, add a with_override(name, value) -> Configuration that returns a new frozen instance, matching the with_* convention used everywhere else. Configuration(overrides={...}) stays the primary path.

  2. For the retry keys, pick one of:

    • Remove them, leaving only the proxy keys Configuration actually drives, and document retry tuning as a ctx.options concern; or
    • Wire them up with a thin bridge — e.g. a factory that reads MAX_RETRY_ATTEMPTS / REQUEST_RETRY_DEFAULT_TIMEOUT off a Configuration and seeds RetryPolicy's constructor defaults — so the keys mean what their names promise.

Both changes are public-API breaks (ConfigurationBuilder is in __all__; the retry keys are public ClassVars) and removing the keys gives up some Java-SDK parity. But parity that silently does nothing is worse than an honestly narrower surface, and removing a redundant builder brings the one outlier back in line with the rest of the SDK.

Acknowledging the current rationale

The well-known keys carry the comment "mirror Java SDK constants where they apply," and the module docstring lists "retry caps" among what Configuration is meant to tune — so the intent was clearly parity with the Java side. The point of revisiting is precisely that "where they apply" is the load-bearing clause: the retry keys do not apply here because this port routes retry tuning through ctx.options, not Configuration. Keeping the constants only preserves the appearance of parity, not the behaviour, which is the kind of gap the conventions would rather see closed than papered over.

Metadata

Metadata

Assignees

No one assigned

    Labels

    documentationImprovements or additions to documentationenhancementNew feature or request

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions