close
Skip to content

Add hierarchy tree diff for plan review#6

Open
silverstein wants to merge 4 commits intom:mainfrom
silverstein:hierarchy-tree-diff
Open

Add hierarchy tree diff for plan review#6
silverstein wants to merge 4 commits intom:mainfrom
silverstein:hierarchy-tree-diff

Conversation

@silverstein
Copy link
Copy Markdown
Contributor

@silverstein silverstein commented Apr 2, 2026

Addresses the structural visibility gap from #3: the flat plan table hides parent-child relationships, making it impossible to see when retiring a category would orphan its children.

What it does

render_category_tree() in lib/helpers.py takes the exported categories and an actions dict, renders an annotated ASCII tree:

Categories
├── Links (23)  ✕ retire  ⚠ 2 children orphaned
│   ├── Blogroll (10)  ⚠ orphaned
│   └── Resources (5)  ⚠ orphaned
├── Personal (89)
├── WordPress (150)
│   ├── Plugins (45)
│   │   ├── Akismet (2)  ✕ retire → plugins
│   │   └── WooCommerce (12)
│   └── Themes (30)
│       └── Full Site Editing (14)  ★ new
└── WP (3)  → merge into wordpress

Retire/merge/create actions are annotated inline. Orphan warnings appear when retiring a parent would leave kept children stranded at root level.

Changes

  • lib/helpers.py: 5 new functions (~280 lines), pure stdlib Python
  • tests/test_helpers.py: 21 new tests (18 tree rendering + 3 orphan detection)
  • AGENTS.md: new "Hierarchy Tree View" section before the flat table, with usage example and actions schema
  • agents/analyze.md: note about including parent_slug for new category suggestions

Edge cases handled

  • Flat sites (all root-level)
  • Deep nesting (4+ levels)
  • Circular parent references (broken and flagged)
  • Missing parent term_id (treated as root, warned)
  • Create-only (no existing categories)
  • Create with nonexistent parent_slug
  • String IDs from WordPress APIs (coerced to int)

All 80 existing + new tests pass.

silverstein and others added 2 commits April 5, 2026 14:40
Addresses the structural visibility gap documented in issue m#3: the flat
plan table hides parent-child relationships, making it impossible to see
when retiring or merging a category would orphan its children.

render_category_tree() in lib/helpers.py takes the exported categories
and an actions dict, then renders an annotated ASCII tree with
box-drawing characters showing merges, retirements, new categories,
and orphan warnings at a glance.

21 new tests covering nesting, annotations, orphan detection, edge
cases (circular parents, missing parents, create-only).

AGENTS.md updated to reference the tree view before the flat table
in the Plan & Descriptions step.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Coerce term_id and parent to int in _build_tree (WordPress APIs may
  return string IDs, causing false "parent missing" warnings)
- Add test for string ID handling
- Add test for circular parent detection
- Move _detect_orphans import to top-level in tests
- Fix test name: test_one_orphan → test_all_children_orphaned
- Fix AGENTS.md example to match actual two-space output format

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@silverstein silverstein force-pushed the hierarchy-tree-diff branch from 2fe7377 to 5122311 Compare April 5, 2026 21:49
The main value of the tree view is orphan detection — the docs example
should demonstrate it, not hide it behind a footnote.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This is looking good. I only have a minor comment, below.

Comment thread lib/helpers.py Outdated
Comment on lines +611 to +616
if synthetic:
label = name
if count:
label += f' ({count})'
else:
label = f'{name} ({count})'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small inconsistency I noticed here: real categories always render as Name (count), even when count is 0, but synthetic ones drop the (0) and just show Name. So a plan that mixes a real empty category with a brand-new one ends up looking like this:

├── 1999 (0)
├── Fresh Pick  ★ new
└── The Good Wife (0)

Same numeric count, two different treatments, and as a reviewer I can't really tell whether Fresh Pick has unknown count, zero count, or "doesn't apply." I tested this against a site of mine that happens to have a few count=0 categories and the ambiguity is real.

I'm wondering if we could just always render (count) for both, even when it's 0? It's a one-liner — drop the if synthetic branch and always do f'{name} ({count})'.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, and thanks for testing it against a real site. You're right that the two treatments are confusing. A bare name next to names with (0) leaves you guessing whether the count is zero or just not applicable.

Fixed in 9f99b3d. Dropped the special-case branch entirely so both real and synthetic nodes always render as Name (count). Also removed the now-unused synthetic variable and updated the example output in AGENTS.md.

Before:

├── 1999 (0)
├── Fresh Pick  ★ new
└── The Good Wife (0)

After:

├── 1999 (0)
├── Fresh Pick (0)  ★ new
└── The Good Wife (0)

All 80 tests pass.

Fixes visual inconsistency where real categories rendered as Name (0) but
synthetic (create action) nodes dropped the count and showed just Name.
This made it unclear whether a new category had zero posts or an unknown
count. Now both render uniformly as Name (0).

Addresses review feedback from @jeherve.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This tests well for me now, it should be good to go!

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