refactor: decompose show() into named stages#714
Merged
Conversation
…tion Decompose the show() god-function (#697). First, behavior-preserving steps: - pass _validate_show_parameters args by keyword so a signature reorder can no longer silently misvalidate one parameter as another - extract _collect_render_commands() and _normalize_title() module helpers No behavior change.
…bar stages Continue decomposing show() (#697), behavior-preserving: - _resolve_coordinate_systems(): CS auto-detection, validation and filtering - _plan_panels(): panel layout (one-per-CS vs one-per-color-key) + ax-count check - _build_legend_params(): LegendParams construction with legend_params overrides - promote the _draw_colorbar closure to a module function taking colorbar_params explicitly (was a ~90-line closure capturing it implicitly) - _layout_pending_colorbars(): the deferred second-pass colorbar layout No behavior change.
…_finalize_panel Extract the inner render-command dispatch loop (#697), behavior-preserving: - _render_panel(): dispatches each queued render command into one panel's axes, returning the wanted elements and per-type wants_* flags - _finalize_panel(): per-panel title / equal-aspect / frame visibility show() is now a compact orchestrator calling named stages. Full test suite passes unchanged (719 passed, 1 skipped), image baselines included. No behavior change.
Post-review cleanups (all behavior-preserving, full suite green 719 passed): - drop dead _draw_colorbar param base_offsets_axes (never read) - collapse the two parallel 4-branch location chains in _draw_colorbar into data-driven lookups (vertical flag + opposite map + getattr on the axis) - extract _should_rasterize() to dedup the images/labels rasterize heuristic - extract _maybe_set_label_colors() for the categorical-color prestep - replace the 5-branch render dispatch in _render_panel with a renderer table keyed by command; graph stays a small special case - pass cs_row instead of four has_* booleans; return a wants dict instead of a four-boolean tuple (removes the parallel-variable sprawl)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #714 +/- ##
==========================================
+ Coverage 77.00% 77.13% +0.13%
==========================================
Files 14 14
Lines 4457 4457
Branches 1036 1023 -13
==========================================
+ Hits 3432 3438 +6
- Misses 660 661 +1
+ Partials 365 358 -7
🚀 New features to boost your workflow:
|
timtreis
changed the title
Follow-up cleanup on the show() decomposition: - Hoist the render-command dispatch dict to a module-level _RENDERERS constant instead of rebuilding it on every _render_panel call. - Derive _VALID_RENDER_CMDS from _RENDER_CMD_TO_CS_FLAG (+ render_graph) so the valid-command set isn't a second hand-maintained list. - Drop the always-true `if "render" in cmd` branch in _collect_render_commands (every valid command starts with "render", and unknown commands already raise above it). - Hoist _finalize_panel out of the per-command loop in _render_panel; it depends only on per-panel values, so it now runs once per panel. Behavior-preserving; no public API change. (Pre-existing mypy errors on this branch are unrelated and unchanged by this commit.)
`_render_panel` iterates render commands whose params are a 5-way RenderParams union. The command string discriminates the concrete type but mypy can't infer that, so narrow with explicit casts at each typed call (_render_graph, _get_wanted_render_elements, _should_rasterize, _maybe_set_label_colors) and type _RENDERERS as `dict[str, Callable[..., None]]` so the dynamic dispatch type-checks. Also fixes an unrelated pre-existing mypy error in utils.py (`_resolve_measure_table` returning Any) by coercing the table name to str; ruff additionally normalized the adjacent spatialdata import block. Both are required for this branch to pass the mypy/ruff pre-commit hooks (mypy follows imports, so the utils.py error blocked any basic.py commit). No runtime behavior change: casts are no-ops and table names are strings.
timtreis
added a commit
that referenced
this pull request
Jun 14, 2026
Builds on the per-panel title fix (#695): before the panel loop, validate that `title`, when a list, has length 1 or exactly num_panels. This: - surfaces the error before any drawing (not mid-render), - rejects an over-long title list (previously silently truncated), and - makes the per-panel IndexError dead code, so the try/except is removed. The error is now a ValueError (the right type for an invalid argument) raised once up front, instead of an IndexError raised per panel mid-render. No test pinned the old exception type. Adds non-visual regression tests for single- and multi-panel title-count validation. Pre-existing mypy error in utils.py (_resolve_measure_table returning Any) is unrelated and unchanged here; it is fixed separately in #714.
timtreis
added a commit
that referenced
this pull request
Jun 14, 2026
Builds on the per-panel title fix (#695): before the panel loop, validate that `title`, when a list, has length 1 or exactly num_panels. This: - surfaces the error before any drawing (not mid-render), - rejects an over-long title list (previously silently truncated), and - makes the per-panel IndexError dead code, so the try/except is removed. The error is now a ValueError (the right type for an invalid argument) raised once up front, instead of an IndexError raised per panel mid-render. No test pinned the old exception type. Adds non-visual regression tests for single- and multi-panel title-count validation. Pre-existing mypy error in utils.py (_resolve_measure_table returning Any) is unrelated and unchanged here; it is fixed separately in #714.
timtreis
added a commit
that referenced
this pull request
Jun 14, 2026
Builds on the per-panel title fix (#695): before the panel loop, validate that `title`, when a list, has length 1 or exactly num_panels. This: - surfaces the error before any drawing (not mid-render), - rejects an over-long title list (previously silently truncated), and - makes the per-panel IndexError dead code, so the try/except is removed. The error is now a ValueError (the right type for an invalid argument) raised once up front, instead of an IndexError raised per panel mid-render. No test pinned the old exception type. Adds non-visual regression tests for single- and multi-panel title-count validation. Pre-existing mypy error in utils.py (_resolve_measure_table returning Any) is unrelated and unchanged here; it is fixed separately in #714.
Integrate #702 (per-panel title fix) and #703 (as_points + fast extent) into the show() decomposition. Conflict resolution in src/spatialdata_plot/pl/basic.py: - #702 up-front title-count validation: kept (after num_panels). The adjacent axes/panel-count check was dropped here because the decomposition already relocated it into _plan_panels. - #702 simplified title selection (dropped the per-panel try/except): applied to the extracted _finalize_panel helper. - #703 had no show()-level render-dispatch changes (as_points is param-driven in render.py, handled inside _render_panel already); its only show()-level change, get_extent -> _get_extent_fast, auto-merged into the extent block and consumes _render_panel's `wants` dict. Also sweeps up two pre-existing #703 lint/type nits in utils.py surfaced by the merge: _fast_extent docstring (ruff D205) and _get_extent_fast Any-return (mypy). Verified: ruff + mypy clean; 109 non-visual show/shapes/labels tests pass.
timtreis
added a commit
that referenced
this pull request
Jun 14, 2026
Integrate #714 (show() decomposition) into the utils.py split. Only utils.py conflicted: - import block: dropped the now-unused `_locate_value` import (it moved to _color.py with the color code that uses it); kept main's `_locate_value` out of utils. - `_fast_extent` docstring: took main's #714 version (D205 fix). basic.py auto-merged: #714's decomposed show()/helpers now import color and validation symbols from _color/_validate (the split's repoints), not utils. Bonus: merging #714 brings its fixes for the pre-existing #703/#705 debt (_resolve_measure_table str-return, _get_extent_fast Any-return, _fast_extent D205), so the branch is now fully ruff + mypy clean (no --no-verify). Verified: no import cycle; ruff + ruff-format + mypy all pass; 410 non-visual tests pass.
This was referenced Jun 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #697.
Decomposes the 644-line
show()god-function into named, single-purpose helpers. Behavior-preserving — no public API change.Extracted helpers
_collect_render_commands·_normalize_title·_resolve_coordinate_systems·_plan_panels·_build_legend_params·_draw_colorbar(promoted from closure) ·_layout_pending_colorbars·_render_panel·_finalize_panel·_should_rasterize·_maybe_set_label_colorsAlso:
_validate_show_parametersnow called by keyword; per-render dispatch is a table;has_*/wants_*boolean sprawl replaced bycs_row+ awantsdict.Metrics (
lizard,basic.py)show()cyclomatic complexityshow()NLOC