Skip to content

Lift the text-append curation helper out of gtdb_ground (#526) - #528

Open
realmarcin wants to merge 1 commit into
curation-history-325from
lift-curation-appender-526
Open

Lift the text-append curation helper out of gtdb_ground (#526)#528
realmarcin wants to merge 1 commit into
curation-history-325from
lift-curation-appender-526

Conversation

@realmarcin

Copy link
Copy Markdown
Contributor

Stacked on #527 (base curation-history-325), which itemises the writers this makes fixable. Review #527 first.

Why it had to move

_append_curation_event was the only code in the repo that adds a CurationEvent without a YAML round-trip — and it lived inside scripts/gtdb_ground.py.

That matters because the nine writers #325 lists as owing a trace are line editors: splitlines(), regex, write_text. record_curation_event appends to a parsed dict, so it does not reach them. And re-dumping is not an option — it would reflow every record they touch, which is why term_remap describes itself as "text-only edits".

Nine private copies would be nine chances to re-hit what this one already knows, each learned from a real failure:

  • - timestamp: starts at column 0, so a naive "next top-level key" scan treats the history's own first item as the next section and inserts the event above the existing history — which the append-only write guard then correctly refuses.
  • curation_history: [] is the same key as curation_history:. Matching only the bare string appends a second one, which PyYAML resolves by keeping the last — silently dropping the existing history.
  • A comment block before the next key belongs to that key, not to the history.

What changed

Now communitymech.curate.curation_event.append_curation_event_text, with curator and width as parameters rather than hardcoded to gtdb_ground.py. The script keeps a thin wrapper, so its curator string stays in one place and tests/test_gtdb_curation_history.py keeps exercising the shared code through its original caller.

One deliberate behaviour change: the library raises ValueError where the script raised SystemExit. SystemExit does not inherit from Exception, so a caller with except Exception would not catch it — it would take the process down mid-sweep, which is precisely what a shared helper must not do. gtdb_ground.py converts at its own boundary, so its CLI behaviour and message are unchanged.

First user

drop_obsolete_go_bp.py, wired and removed from _OWED (9 → 8). Deletions are where a trace matters most — the removed lines are simply not there afterwards, so "what did this and why" is least recoverable from the record itself.

The canary had to be built, not run

--dry-run against the corpus reports 0 files, because that cleanup already ran. That is the exact shape of check that passes while persisting nothing.

So the test constructs a record that does carry a droppable annotation, runs the script in a temp tree the way a batch would, and reads the file back off disk. Reverting the wiring turns it red; the other nine tests stay green, which is why it is worth having separately.

Checks

  • uv run pytest tests/ — 2391 passed, 16 skipped
  • just lint, just validate-strict — exit 0
  • Ten library tests exercise the helper with a curator that is not gtdb_ground.py — the case the existing tests cannot reach, since they only call through the original caller

Closes #526. Advances #325 (9 owed → 8).

🤖 Generated with Claude Code

`_append_curation_event` was the only code here that adds a CurationEvent
without a YAML round-trip, and it lived inside one script. The nine writers #325
lists as owing a trace are line editors — splitlines, regex, write_text — so
`record_curation_event`, which appends to a parsed dict, does not reach them.
Nine private copies would be nine chances to re-hit what this one already knows:
that `- timestamp:` is column-0 so a naive scan inserts the event above the
existing history, that `curation_history: []` is the same key and matching the
bare string appends a second one PyYAML silently drops, and that a trailing
comment block belongs to the next key.

Now `communitymech.curate.curation_event.append_curation_event_text`, with
curator and width as parameters rather than hardcoded. gtdb_ground keeps a thin
wrapper so its curator string stays in one place and the existing tests keep
exercising the shared code through its original caller.

One behaviour change on purpose: the library raises ValueError where the script
raised SystemExit. SystemExit does not inherit from Exception, so a caller with
`except Exception` would not catch it — it would take the process down
mid-sweep, which is exactly what a shared helper must not do. gtdb_ground
converts at its own boundary, so its CLI message is unchanged.

drop_obsolete_go_bp.py is wired as the first user, and comes off the owed list.
Deletions are where a trace matters most: the removed lines are simply not there
afterwards.

The canary needed building rather than running. The corpus has no droppable
annotations left, so a dry run reports zero and proves nothing — the shape of
check that passes while persisting nothing. The test constructs a record that
does have one, runs the script in a temp tree the way a batch would, and reads
the file back off disk. Reverting the wiring reddens it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant