diff --git a/.issueflows/04-designs-and-guides/cellpycell-di-restructuring.md b/.issueflows/04-designs-and-guides/cellpycell-di-restructuring.md new file mode 100644 index 00000000..48051571 --- /dev/null +++ b/.issueflows/04-designs-and-guides/cellpycell-di-restructuring.md @@ -0,0 +1,59 @@ +# ADR: dependency-injection restructuring of CellpyCell (#520, V2-09 tail) + +**Status:** accepted (2026-07-17) · **Issue:** [#520](https://github.com/jepegit/cellpy/issues/520) · **Epic:** #402 (V2-09) + +## Context + +`CellpyCell.__init__` historically built everything itself. After the +mechanical extractions (capacity curves #509, exporters #518, split/drop +#519), the class still *constructs* three collaborator kinds inline: + +1. **The core seam** — `OldCellpyCellCore` (legacy bridge) or, under the + `native_schema` flag (#511), `NativeCellpyCellCore`. The core owns the + `Data` object and runs the step/summary engine. +2. **The instrument factory** — `register_instrument_readers()` builds + `ds.generate_default_factory()`; `set_instrument()` then creates + `self.loader_class` from it. +3. **Config-derived options** — reader options are copied from `prms` at + init (`self.sep`, `self.ensure_step_table`, data dirs, …); units come + from `get_cellpy_units(cellpy_units)`. + +Only (1) and (2) are *behavioural collaborators* worth injecting: tests and +embedders need to substitute them (fake engine, fake loader registry) +without monkeypatching. (3) is plain data already parameterizable. + +## Decision + +**Own vs receive:** + +| Collaborator | `__init__` policy | +|---|---| +| core seam | **receive** via `core=` (default: built from the `native_schema` flag as today). An injected `core` wins; the flag then only documents intent. | +| instrument factory | **receive** via `instrument_factory=` (default: `ds.generate_default_factory()`; built lazily in `register_instrument_readers()` only when none was injected). | +| config snapshot, units, headers | **own** — plain data, already overridable via existing parameters (`cellpy_units=`, prms). Full config inversion is a non-goal here. | + +**Wiring of the extracted modules** (capacity_curves / exporters.tabular / +slicing): stays as instance-first functions plus thin delegates. No service +locator, no registry — the delegate methods *are* the wiring, and subclass +dispatch works because cross-calls go through the instance. + +**`register_instrument_readers()`** keeps its public name but becomes +idempotent-with-injection: it only builds the default factory when +`self.instrument_factory` is `None`. Calling it after injecting a factory is +a no-op instead of silently discarding the injected one. + +## Non-goals + +- Splitting `CellpyCell` into multiple classes (that is the Phase-3 / + native-runtime discussion, not V2-09). +- Inverting `prms` / config access. +- Changing `set_instrument()` semantics or the loader plug-in story (#210). +- Removing the near-dead `_cap_mod_*` pair — decided here: **keep** them as + moved (#518) until a deprecation sweep; they are test-pinned and harmless. + +## Acceptance + +- `tests/test_slim.py` (core-seam acceptance) stays green throughout. +- Defaults produce byte-identical behaviour (full suite green). +- New pins: injecting a custom core is used by the pipeline; injecting a + custom instrument factory is used by `set_instrument`. diff --git a/HISTORY.md b/HISTORY.md index 240c8215..76c21baf 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -2,6 +2,13 @@ ## [Unreleased] +* Dependency-injection tail of V2-09 (#520): `CellpyCell(core=..., + instrument_factory=...)` — the core seam and the loader registry are now + constructor-injectable (defaults unchanged); + `register_instrument_readers()` keeps an injected factory instead of + silently rebuilding. ADR: + `.issueflows/04-designs-and-guides/cellpycell-di-restructuring.md`. + * Split/drop-cycle helpers extracted from `cellreader.py` into `cellpy.readers.slicing` (#519, V2-09 follow-up): `split`, `split_many`, `drop_from`/`drop_to`, `from_cycle`/`to_cycle`, `drop_edges`, diff --git a/cellpy/readers/cellreader.py b/cellpy/readers/cellreader.py index dc14c023..a3177368 100644 --- a/cellpy/readers/cellreader.py +++ b/cellpy/readers/cellreader.py @@ -212,6 +212,8 @@ def __init__( output_units=None, debug=False, native_schema=False, + core=None, + instrument_factory=None, ): """ Args: @@ -226,6 +228,13 @@ def __init__( cellpy_units (dict): sent to cellpy.parameters.internal_settings.get_cellpy_units output_units (dict): sent to cellpy.parameters.internal_settings.get_default_output_units debug (bool): set to True if you want to see debug messages. + core (CellpyCellCore): injected core seam (issue #520, DI). When + given it is used as-is (it owns the ``Data`` object and runs + the step/summary engine); when None the default is built from + the ``native_schema`` flag. + instrument_factory (InstrumentFactory): injected loader registry + (issue #520, DI). When None, + ``register_instrument_readers()`` builds the default factory. native_schema (bool): opt-in feature flag (issue #511, V2-11). When True, frames are kept in native cellpy-core column names and the polars engine runs directly (no legacy rename @@ -250,9 +259,12 @@ def __init__( # initialize() creates the cellpy ``ds.Data`` it expects (the data # property reads/writes ``self.core._data``). Under the ``native_schema`` # opt-in (#511) the legacy bridge is replaced by the rename-free - # pandas<->polars adapter. + # pandas<->polars adapter. An injected ``core`` (#520, DI) wins over + # both defaults. self.native_schema = bool(native_schema) - if self.native_schema: + if core is not None: + self.core = core + elif self.native_schema: from cellpy.readers.native_core import NativeCellpyCellCore self.core = NativeCellpyCellCore(initialize=False, debug=debug) @@ -318,7 +330,7 @@ def __init__( self.headers_normal = headers_normal self.headers_summary = headers_summary self.headers_step_table = headers_step_table - self.instrument_factory = None + self.instrument_factory = instrument_factory # injected (#520) or None self.register_instrument_readers() self.set_instrument() # - units used by cellpy @@ -592,9 +604,14 @@ def __register_external_readers(self): return def register_instrument_readers(self): - """Register instrument readers.""" + """Register instrument readers. - self.instrument_factory = ds.generate_default_factory() + Builds the default factory only when none is set — an injected + ``instrument_factory`` (#520, DI) is kept as-is. Set + ``self.instrument_factory = None`` first to force a rebuild. + """ + if self.instrument_factory is None: + self.instrument_factory = ds.generate_default_factory() # instruments = find_all_instruments() # for instrument_id, instrument in instruments.items(): # self.instrument_factory.register_builder(instrument_id, instrument) diff --git a/tests/test_slim.py b/tests/test_slim.py index 4d6dfc36..d2d0e206 100644 --- a/tests/test_slim.py +++ b/tests/test_slim.py @@ -107,3 +107,45 @@ def test_make_summary_selector_kwargs_deprecated(cpi, parameters): cpi.make_summary(selector_type="non-cv") assert cpi.data.summary is not None assert not cpi.data.summary.empty + + +# --- dependency injection (#520, V2-09 tail; see the ADR in +# --- .issueflows/04-designs-and-guides/cellpycell-di-restructuring.md) ------ +def test_injected_core_is_used_by_pipeline(parameters): + """An injected core seam owns the Data object and runs the pipeline.""" + from cellpy import cellreader + + core = OldCellpyCellCore(initialize=False) + cpi = cellreader.CellpyCell(core=core) + assert cpi.core is core + + cpi.set_instrument("arbin_res") + cpi.from_raw(parameters.res_file_path) + cpi.make_step_table() + + assert cpi.core is core + assert core._data is cpi.data + assert not cpi.data.steps.empty + + +def test_injected_instrument_factory_is_used(): + """An injected factory is used by set_instrument and never rebuilt.""" + from cellpy import cellreader + from cellpy.readers import data_structures as ds + + factory = ds.generate_default_factory() + calls = [] + original_create = factory.create + + def counting_create(instrument, **kwargs): + calls.append(instrument) + return original_create(instrument, **kwargs) + + factory.create = counting_create + cpi = cellreader.CellpyCell(instrument_factory=factory) + assert cpi.instrument_factory is factory + assert calls # __init__ -> set_instrument went through the injected factory + + # register_instrument_readers keeps the injected factory (idempotent) + cpi.register_instrument_readers() + assert cpi.instrument_factory is factory