Skip to content

Add nodes from dict - #762

Merged
nwlandry merged 5 commits into
devfrom
add-nodes-from-dict
Oct 2, 2026
Merged

nwlandry merged 5 commits into
devfrom
add-nodes-from-dict

Conversation

@nwlandry

Copy link
Copy Markdown
Collaborator

This allows users to add nodes from a dict-of-dicts.

@codecov

codecov Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.56%. Comparing base (93b95ed) to head (3a1690d).
⚠️ Report is 1 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #762      +/-   ##
==========================================
+ Coverage   94.53%   94.56%   +0.02%     
==========================================
  Files          63       63              
  Lines        5146     5170      +24     
==========================================
+ Hits         4865     4889      +24     
  Misses        281      281              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nwlandry
nwlandry requested a review from leotrs September 28, 2026 14:05

@leotrs leotrs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, closes #752 cleanly. Docstring examples in particular are clear and cover the four call shapes explicitly. Approving with a couple of edge cases worth thinking about.

Ambiguity worth flagging

The dispatch heuristic isinstance(n, tuple) and len(n) == 2 and isinstance(n[1], dict) means these two patterns collide:

# Intent A: add a single node whose ID happens to be a 2-tuple with a dict value
H.add_nodes_from([("my_node", {"role": "root"})])

# Intent B: add node "my_node" with attribute {"role": "root"}
H.add_nodes_from([("my_node", {"role": "root"})])

They look identical to the caller and now always resolve as Intent B. The old try/except path had the same ambiguity for hashable 2-tuples of the same shape, so this is not a regression, just an inherited limitation. If it bites someone, the workaround is add_node((tuple_id, some_dict)). Worth a line in the docstring so a future reader knows why the heuristic looks the way it does.

Silent misinterpretation when dict values are not dicts

If a caller passes {1: None} (say from {n: metadata.get(n) for n in nodes} where lookups can miss), the code takes the .items() path and gets (1, None) tuples. Since None isn't a dict, each falls into the else branch, and n becomes the whole (1, None) tuple, which then becomes the node ID. So instead of a Hypergraph with node 1, you get a Hypergraph with node (1, None). Same story for any non-dict value.

Two options:

  • Silently coerce non-dict values to {} (accept the sloppy input, add the node without attributes).
  • Raise a TypeError up front when a dict is passed with any non-dict value, naming the offender.

Either is fine. I'd lean toward the raise for the same "loud failure at the boundary" reason we discussed on the AHORN loader. Not blocking.

Small thing

The test could gain one line for the new dict path receiving pre-existing nodes (e.g., call H.add_nodes_from({0: {"weight": 2}}) after node 0 already exists to verify attribute update behavior). Non-blocking.

@nwlandry
nwlandry merged commit 9d6e379 into dev Oct 2, 2026
19 checks passed
@nwlandry
nwlandry deleted the add-nodes-from-dict branch October 2, 2026 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants