Skip to content

Commit 2c474ad

Browse files
AlexMikhalevclaude
andcommitted
fix(rolegraph-py): address code review findings
- Simplify __repr__ to avoid calling get_graph_stats() on every repr (was O(n) for large graphs, now just shows role + document count) - Fix LogicalOperator type stub to use LogicalOperator type instead of int - Strengthen test_is_all_terms_connected_by_path to assert expected True for co-occurring terms instead of only checking return type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent f9ad14d commit 2c474ad

3 files changed

Lines changed: 12 additions & 10 deletions

File tree

crates/terraphim_rolegraph_py/python/terraphim_rolegraph/__init__.pyi

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ class GraphStats:
7777
class LogicalOperator:
7878
"""Logical operator for combining search terms."""
7979

80-
And: int
81-
Or: int
80+
And: "LogicalOperator"
81+
Or: "LogicalOperator"
8282

8383
class RoleGraph:
8484
"""A knowledge graph for a specific role."""

crates/terraphim_rolegraph_py/python/tests/test_rolegraph.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -121,11 +121,16 @@ def test_find_matching_node_ids(self, populated_rolegraph):
121121
assert len(ids) >= 2
122122

123123
def test_is_all_terms_connected_by_path(self, populated_rolegraph):
124-
# Terms that co-occur in the same document should be connected
125-
result = populated_rolegraph.is_all_terms_connected_by_path(
124+
# "machine learning" and "deep learning" co-occur in doc1, so they are connected
125+
assert populated_rolegraph.is_all_terms_connected_by_path(
126126
"machine learning deep learning"
127+
) is True
128+
# A term not in the thesaurus produces no nodes, so connectivity is vacuously true;
129+
# verify a single known term still returns a bool
130+
assert isinstance(
131+
populated_rolegraph.is_all_terms_connected_by_path("machine learning"),
132+
bool,
127133
)
128-
assert isinstance(result, bool)
129134

130135

131136
class TestGraphStats:

crates/terraphim_rolegraph_py/src/lib.rs

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -477,13 +477,10 @@ impl PyRoleGraph {
477477
// -- Dunder methods -----------------------------------------------------
478478

479479
fn __repr__(&self) -> String {
480-
let stats = self.inner.get_graph_stats();
481480
format!(
482-
"RoleGraph(role='{}', nodes={}, edges={}, documents={})",
481+
"RoleGraph(role='{}', documents={})",
483482
self.inner.role.original,
484-
stats.node_count,
485-
stats.edge_count,
486-
stats.document_count
483+
self.inner.document_count()
487484
)
488485
}
489486

0 commit comments

Comments
 (0)