Skip to content

Commit 96ccb6d

Browse files
committed
refactor: fix SonarCloud maintainability issues in diagram package
- Use pytest.approx for float comparisons in _split_length tests - Rename unused variables to _ in _prepare_svg tests - Remove unused 'getter' parameter from _extract_transitions_from_state and _extract_all_transitions - Reduce cognitive complexity of _create_edges (30→~10) by extracting _create_single_edge, _resolve_edge_endpoints, and _build_edge_label - Reduce cognitive complexity of _render_states (17→~12) by extracting _place_extra_nodes static method
1 parent 73e1218 commit 96ccb6d

3 files changed

Lines changed: 111 additions & 82 deletions

File tree

statemachine/contrib/diagram/extract.py

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ def _extract_state(
101101
)
102102

103103

104-
def _extract_transitions_from_state(state: "State", getter) -> List[DiagramTransition]:
104+
def _extract_transitions_from_state(state: "State") -> List[DiagramTransition]:
105105
"""Extract transitions from a single state (non-recursive)."""
106106
result: List[DiagramTransition] = []
107107
for transition in state.transitions:
@@ -122,17 +122,17 @@ def _extract_transitions_from_state(state: "State", getter) -> List[DiagramTrans
122122
return result
123123

124124

125-
def _extract_all_transitions(states, getter) -> List[DiagramTransition]:
125+
def _extract_all_transitions(states) -> List[DiagramTransition]:
126126
"""Recursively extract transitions from all states."""
127127
result: List[DiagramTransition] = []
128128
for state in states:
129-
result.extend(_extract_transitions_from_state(state, getter))
129+
result.extend(_extract_transitions_from_state(state))
130130
if state.states:
131-
result.extend(_extract_all_transitions(state.states, getter))
131+
result.extend(_extract_all_transitions(state.states))
132132
for history_state in getattr(state, "history", []):
133-
result.extend(_extract_transitions_from_state(history_state, getter))
133+
result.extend(_extract_transitions_from_state(history_state))
134134
if history_state.states: # pragma: no cover
135-
result.extend(_extract_all_transitions(history_state.states, getter))
135+
result.extend(_extract_all_transitions(history_state.states))
136136
return result
137137

138138

@@ -242,7 +242,7 @@ class itself thanks to the metaclass. Active-state highlighting is only
242242
for state in machine.states:
243243
states.append(_extract_state(state, machine, getter, active_values))
244244

245-
transitions = _extract_all_transitions(machine.states, getter)
245+
transitions = _extract_all_transitions(machine.states)
246246

247247
compound_ids = _collect_compound_ids(states)
248248
bidir_ids = _collect_bidirectional_compound_ids(transitions, compound_ids)

statemachine/contrib/diagram/renderers/dot.py

Lines changed: 98 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -162,15 +162,9 @@ def _render_states(
162162
atomic_subgraph.add_node(self._create_atomic_node(state))
163163
has_atomic = True
164164

165-
# Anchor nodes from the parent compound are co-located with real states
166-
# when possible. If there are no atomic states at this level (e.g. a
167-
# parallel state with only compound children), add them directly to the
168-
# parent graph to avoid creating an empty cluster that wastes space.
169-
if extra_nodes:
170-
target = atomic_subgraph if has_atomic else parent_graph
171-
for node in extra_nodes:
172-
target.add_node(node)
173-
has_atomic = has_atomic or (target is atomic_subgraph)
165+
has_atomic = self._place_extra_nodes(
166+
extra_nodes, atomic_subgraph, parent_graph, has_atomic
167+
)
174168

175169
if has_atomic:
176170
parent_graph.add_subgraph(atomic_subgraph)
@@ -180,6 +174,28 @@ def _render_states(
180174
if not state.children:
181175
self._add_transitions_for_state(state, transitions, parent_graph)
182176

177+
@staticmethod
178+
def _place_extra_nodes(
179+
extra_nodes: Optional[List[pydot.Node]],
180+
atomic_subgraph: pydot.Subgraph,
181+
parent_graph: "pydot.Dot | pydot.Subgraph",
182+
has_atomic: bool,
183+
) -> bool:
184+
"""Place anchor nodes from the parent compound into the graph.
185+
186+
Co-locates them with real states when possible. If there are no atomic
187+
states at this level (e.g. a parallel state with only compound children),
188+
adds them directly to the parent graph to avoid an empty cluster.
189+
190+
Returns the updated ``has_atomic`` flag.
191+
"""
192+
if not extra_nodes:
193+
return has_atomic
194+
target = atomic_subgraph if has_atomic else parent_graph
195+
for node in extra_nodes:
196+
target.add_node(node)
197+
return has_atomic or (target is atomic_subgraph)
198+
183199
def _render_initial_arrow(
184200
self,
185201
initial_state: DiagramState,
@@ -438,65 +454,78 @@ def _create_edges(self, transition: DiagramTransition) -> List[pydot.Edge]:
438454
cond = ", ".join(transition.guards)
439455
cond_html = f"<br/>[{_escape_html(cond)}]" if cond else ""
440456

441-
edges = []
442-
for i, target_id in enumerate(target_ids):
443-
extra: Dict[str, str] = {}
444-
source_is_compound = transition.source in self._compound_ids
445-
target_is_compound = target_id is not None and target_id in self._compound_ids
457+
return [
458+
self._create_single_edge(transition, target_id, i, cond_html)
459+
for i, target_id in enumerate(target_ids)
460+
]
446461

447-
if source_is_compound:
448-
extra["ltail"] = f"cluster_{transition.source}"
449-
if target_is_compound:
450-
extra["lhead"] = f"cluster_{target_id}"
451-
452-
has_substates = source_is_compound or target_is_compound
453-
event_text = _escape_html(transition.event) if i == 0 else ""
454-
455-
if target_id is not None:
456-
dst = self._state_node_id(target_id)
457-
else:
458-
dst = self._state_node_id(transition.source)
459-
460-
src = self._state_node_id(transition.source)
461-
462-
# For compound states in bidirectional pairs, route outgoing edges
463-
# through _anchor_out and incoming through _anchor_in so Graphviz
464-
# places them at different physical positions inside the cluster.
465-
if source_is_compound and transition.source in self._compound_bidir_ids:
466-
src = self._compound_edge_anchor(transition.source, "out")
467-
extra["ltail"] = f"cluster_{transition.source}"
468-
if target_is_compound and target_id in self._compound_bidir_ids:
469-
dst = self._compound_edge_anchor(target_id, "in")
470-
extra["lhead"] = f"cluster_{target_id}"
471-
472-
if event_text or (cond_html and i == 0):
473-
label_content = f"{event_text}{cond_html}" if i == 0 else ""
474-
font_size = self.config.transition_font_size
475-
html_label = (
476-
f'<<table border="0" cellborder="0" cellspacing="0" cellpadding="0">'
477-
f'<tr><td cellpadding="4">'
478-
f'<font point-size="{font_size}">{label_content}</font>'
479-
f"</td></tr>"
480-
f'<tr><td cellpadding="1"></td></tr>'
481-
f"</table>>"
482-
)
483-
else:
484-
html_label = ""
485-
486-
# With dedicated anchor nodes placed inside compound clusters,
487-
# the midpoint of anchor→dst falls outside the cluster boundary,
488-
# so the standard label position is correct along the visible edge.
489-
# Use label for all edges (xlabel causes floating/displaced labels).
490-
label_key = "label"
491-
492-
edges.append(
493-
pydot.Edge(
494-
src,
495-
dst,
496-
**{label_key: html_label},
497-
minlen=2 if has_substates else 1,
498-
**extra,
499-
)
500-
)
462+
def _create_single_edge(
463+
self,
464+
transition: DiagramTransition,
465+
target_id: Optional[str],
466+
index: int,
467+
cond_html: str,
468+
) -> pydot.Edge:
469+
"""Create a single pydot.Edge for one target of a transition."""
470+
src, dst, extra = self._resolve_edge_endpoints(transition, target_id)
471+
has_substates = bool(extra)
472+
html_label = self._build_edge_label(transition.event, cond_html, index)
473+
474+
return pydot.Edge(
475+
src,
476+
dst,
477+
label=html_label,
478+
minlen=2 if has_substates else 1,
479+
**extra,
480+
)
501481

502-
return edges
482+
def _resolve_edge_endpoints(
483+
self,
484+
transition: DiagramTransition,
485+
target_id: Optional[str],
486+
) -> "tuple[str, str, Dict[str, str]]":
487+
"""Resolve source/destination node IDs and cluster attributes for an edge."""
488+
extra: Dict[str, str] = {}
489+
source_is_compound = transition.source in self._compound_ids
490+
target_is_compound = target_id is not None and target_id in self._compound_ids
491+
492+
if source_is_compound:
493+
extra["ltail"] = f"cluster_{transition.source}"
494+
if target_is_compound:
495+
extra["lhead"] = f"cluster_{target_id}"
496+
497+
dst = (
498+
self._state_node_id(target_id)
499+
if target_id is not None
500+
else self._state_node_id(transition.source)
501+
)
502+
src = self._state_node_id(transition.source)
503+
504+
# For compound states in bidirectional pairs, route outgoing edges
505+
# through _anchor_out and incoming through _anchor_in so Graphviz
506+
# places them at different physical positions inside the cluster.
507+
if source_is_compound and transition.source in self._compound_bidir_ids:
508+
src = self._compound_edge_anchor(transition.source, "out")
509+
extra["ltail"] = f"cluster_{transition.source}"
510+
if target_is_compound and target_id in self._compound_bidir_ids:
511+
dst = self._compound_edge_anchor(target_id, "in")
512+
extra["lhead"] = f"cluster_{target_id}"
513+
514+
return src, dst, extra
515+
516+
def _build_edge_label(self, event: str, cond_html: str, index: int) -> str:
517+
"""Build the HTML label for a transition edge."""
518+
event_text = _escape_html(event) if index == 0 else ""
519+
if not event_text and not (cond_html and index == 0):
520+
return ""
521+
522+
label_content = f"{event_text}{cond_html}" if index == 0 else ""
523+
font_size = self.config.transition_font_size
524+
return (
525+
f'<<table border="0" cellborder="0" cellspacing="0" cellpadding="0">'
526+
f'<tr><td cellpadding="4">'
527+
f'<font point-size="{font_size}">{label_content}</font>'
528+
f"</td></tr>"
529+
f'<tr><td cellpadding="1"></td></tr>'
530+
f"</table>>"
531+
)

tests/test_contrib_diagram.py

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -700,28 +700,28 @@ def test_split_pt(self):
700700
from statemachine.contrib.diagram.sphinx_ext import _split_length
701701

702702
value, unit = _split_length("702pt")
703-
assert value == 702.0
703+
assert value == pytest.approx(702.0)
704704
assert unit == "pt"
705705

706706
def test_split_px(self):
707707
from statemachine.contrib.diagram.sphinx_ext import _split_length
708708

709709
value, unit = _split_length("100px")
710-
assert value == 100.0
710+
assert value == pytest.approx(100.0)
711711
assert unit == "px"
712712

713713
def test_split_float(self):
714714
from statemachine.contrib.diagram.sphinx_ext import _split_length
715715

716716
value, unit = _split_length("12.5em")
717-
assert value == 12.5
717+
assert value == pytest.approx(12.5)
718718
assert unit == "em"
719719

720720
def test_split_no_match(self):
721721
from statemachine.contrib.diagram.sphinx_ext import _split_length
722722

723723
value, unit = _split_length("auto")
724-
assert value == 0.0
724+
assert value == pytest.approx(0.0)
725725
assert unit == "auto"
726726

727727

@@ -774,7 +774,7 @@ def test_strips_xml_prologue(self):
774774
b"<circle/></svg>"
775775
)
776776
directive = self._make_directive()
777-
svg_tag, w, h = directive._prepare_svg(svg_bytes)
777+
svg_tag, _, _ = directive._prepare_svg(svg_bytes)
778778

779779
assert not svg_tag.startswith("<?xml")
780780
assert svg_tag.startswith("<svg")
@@ -800,7 +800,7 @@ def test_removes_fixed_dimensions(self):
800800
def test_handles_no_dimensions(self):
801801
svg_bytes = b'<svg viewBox="0 0 100 50"><rect/></svg>'
802802
directive = self._make_directive()
803-
svg_tag, w, h = directive._prepare_svg(svg_bytes)
803+
_, w, h = directive._prepare_svg(svg_bytes)
804804

805805
assert w == ""
806806
assert h == ""

0 commit comments

Comments
 (0)