Summary
PlotAccessor.show (src/spatialdata_plot/pl/basic.py:1250-1894, ~644 lines) is a god-function interleaving 8 distinct responsibilities with a doubly-nested render loop. Decomposing it into named stages would make each independently readable/testable and remove the reassigned-local-variable hazards that already produced two bugs.
The 8 interleaved responsibilities
- Arg validation / deprecation (~L1370–1448)
- Render-command extraction (~L1418–1441)
- Coordinate-system resolution (~L1462–1503)
- Panel-layout planning (~L1505–1538)
- Canvas / legend-param setup (~L1540–1575)
- Per-panel render dispatch loop (~L1669–1854, ~185 lines, 4 levels deep)
- Second-pass colorbar layout engine (~L1577–1878)
- Save / show / return (~L1880–1894)
No stage is independently testable; locals (ax, sdata, cs, coordinate_systems) are reassigned across stages — which is exactly what enabled the two bugs filed alongside this audit (leaked cs; per-command title block).
Suggested decomposition
Extract pure-ish stage helpers so show becomes a ~60-line orchestrator:
_collect_render_commands(plotting_tree)_resolve_coordinate_systems(...)_plan_panels(...)_build_legend_params(...)_render_panel(...) (the inner loop body)_finalize_panel(ax, i, title, ...)_layout_pending_colorbars(...) (note: _draw_colorbar is currently a ~90-line closure that captures only colorbar_params — promote to a module function taking it explicitly)
Also: the call to _validate_show_parameters passes 24 positional args (basic.py:1379) that must mirror the show signature exactly — switch to keyword args so a reorder can't silently misvalidate (e.g. dpi validated as fig).
Sequencing
Do this as mechanical, behavior-preserving extractions (one stage per commit), and after the two companion bug fixes land, so the extracted stages don't enshrine the bugs.
Risk / effort
Effort: high · Risk: medium — guard with the existing image baselines (CI). Also flatten the has_*/wants_* boolean sprawl in the dispatch loop (8 hand-threaded booleans) into a small dispatch table while restructuring.
Part of a maintainability/refactor audit of main.
Summary
PlotAccessor.show(src/spatialdata_plot/pl/basic.py:1250-1894, ~644 lines) is a god-function interleaving 8 distinct responsibilities with a doubly-nested render loop. Decomposing it into named stages would make each independently readable/testable and remove the reassigned-local-variable hazards that already produced two bugs.The 8 interleaved responsibilities
No stage is independently testable; locals (
ax,sdata,cs,coordinate_systems) are reassigned across stages — which is exactly what enabled the two bugs filed alongside this audit (leakedcs; per-command title block).Suggested decomposition
Extract pure-ish stage helpers so
showbecomes a ~60-line orchestrator:_collect_render_commands(plotting_tree)_resolve_coordinate_systems(...)_plan_panels(...)_build_legend_params(...)_render_panel(...)(the inner loop body)_finalize_panel(ax, i, title, ...)_layout_pending_colorbars(...)(note:_draw_colorbaris currently a ~90-line closure that captures onlycolorbar_params— promote to a module function taking it explicitly)Also: the call to
_validate_show_parameterspasses 24 positional args (basic.py:1379) that must mirror theshowsignature exactly — switch to keyword args so a reorder can't silently misvalidate (e.g.dpivalidated asfig).Sequencing
Do this as mechanical, behavior-preserving extractions (one stage per commit), and after the two companion bug fixes land, so the extracted stages don't enshrine the bugs.
Risk / effort
Effort: high · Risk: medium — guard with the existing image baselines (CI). Also flatten the
has_*/wants_*boolean sprawl in the dispatch loop (8 hand-threaded booleans) into a small dispatch table while restructuring.Part of a maintainability/refactor audit of
main.