Skip to content

refactor: introduce ensure_no_reserved_kwargs() helper for **kwargs collision detection #466

Description

@murray-ds

Problem

10 of 15 plot modules splat **kwargs into a downstream call after explicitly setting one or more named kwargs that the caller could also supply. When a caller passes one of those reserved keys, Python raises TypeError: got multiple values for keyword argument 'X' from the framework call — a generic error that doesn't explain why the function reserved that key.

This pattern almost certainly recurs outside plots/ (anywhere a public API takes **kwargs and forwards them while also setting some keys directly).

Audit (openretailscience/plots/)

File Colliding keys (raise TypeError if passed via **kwargs)
period_on_period.py linestyle, color, zorder, label
time.py legend
histogram.py kind, legend
waterfall.py x, y, legend, bottom
area.py kind, legend
index.py barh path: zorder, left, y, height; pandas path: legend, left, zorder
bar.py kind, y
broken_timeline.py facecolors (the natural matplotlib name a user would reach for)
venn.py set_labels, subset_sizes, subset_labels, set_colors
price.py c

Safe / non-applicable: line.py (filters via comprehension), scatter.py (uses dict-merge so kwargs silently override the function's reserved keys — a related bug class worth flagging), cohort.py, heatmap.py, tree_diagram.py (no splat).

Example failure

period_on_period.plot(df, x_col=\"date\", value_col=\"sales\", periods=periods,
                     linestyle=\"--\")
# TypeError: ax.plot() got multiple values for keyword argument 'linestyle'
# raised mid-loop, after some periods may already have rendered onto a
# caller-supplied ax

The caller has no way to know linestyle is set per-period by the function as part of the visual ramp — the error names the key but not the reason.

Proposal

Add a helper to openretailscience/utils/validation.py (per CLAUDE.md: "Shared validation functions belong in `openretailscience/utils/validation.py`"):

```python
def ensure_no_reserved_kwargs(
kwargs: dict[str, object],
reserved: Iterable[str],
func_name: str,
) -> None:
"""Raise TypeError if any reserved key appears in kwargs.

Use at the public API boundary of functions that splat **kwargs into a
downstream call while also setting some of those keys explicitly.
\"\"\"

```

The helper goes in `utils/validation.py` because the pattern isn't plot-specific — any public function that forwards `**kwargs` while reserving some keys can use it.

Acceptance Criteria

  • `ensure_no_reserved_kwargs()` added to `openretailscience/utils/validation.py`
  • Unit tests for the helper covering: empty kwargs, no overlap, single overlap, multiple overlaps, and the exact error-message format
  • All 10 vulnerable plot modules above migrated to call the helper at the start of `plot()` (before any axes are constructed, so caller-supplied `ax` is never mutated on failure)
  • `scatter.py`'s dict-merge case also migrated (silent override → explicit raise)
  • Stale docstrings updated where they reference a pre-refactor base function (e.g. `period_on_period.plot` docstring says "passed to the base line plot function" but the refactor sends them straight to `ax.plot()`)
  • All existing tests pass; regression tests added for at least one representative collision per migrated module via `pytest.mark.parametrize`

Out of scope

  • Deciding which kwargs should be forward-safe per module (each module's PR can decide whether to additionally whitelist `marker`, `alpha`, etc. or just reserve the conflicting set).
  • Codebase-wide audit outside `plots/` — flag follow-ups as separate issues if found.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    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