feat: Preserve separate GEDCOM NOTE records through roundtrip#584
feat: Preserve separate GEDCOM NOTE records through roundtrip#584isaacschepp wants to merge 28 commits intomainfrom
Conversation
Introduces NoteList type ([]string with custom YAML marshal/unmarshal) that accepts both scalar strings and YAML sequences, ensuring backwards compatibility with existing archives. Import now appends separate notes instead of concatenating with "\n\n". Export emits each note as a separate GEDCOM NOTE record, preserving original note boundaries through roundtrip. 28 files changed across go-glx and glx packages. All existing tests pass with mechanical updates (string → NoteList wrapping). Fixes #487
Deploying genealogix with
|
| Latest commit: |
e16dadd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3d67d099.genealogix.pages.dev |
| Branch Preview URL: | https://fix-multi-note-roundtrip.genealogix.pages.dev |
There was a problem hiding this comment.
Pull request overview
This PR aims to preserve distinct GEDCOM NOTE record boundaries through an import → YAML → export roundtrip by replacing the single Notes string field with a multi-value NoteList across the GLX data model, while keeping YAML backwards compatibility.
Changes:
- Replace
Notes stringwithNotes NoteListon core entity types and update call sites (append, emptiness checks, string rendering). - Add
NoteListYAML marshal/unmarshal logic plus unit tests for backwards-compatible scalar-vs-sequence YAML representations. - Update GEDCOM import/export and related tests to use the new
NoteListtype.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| go-glx/types.go | Switch entity Notes fields from string to NoteList and update content checks. |
| go-glx/note_list.go | Introduce NoteList with custom YAML marshal/unmarshal and helper methods. |
| go-glx/note_list_test.go | Add unit tests for NoteList YAML behavior and helpers. |
| go-glx/gedcom_source.go | Update source import to store notes as NoteList instead of concatenated strings. |
| go-glx/gedcom_shared.go | Update event detail extraction to append note entries. |
| go-glx/gedcom_repository.go | Update repository import to store notes as NoteList. |
| go-glx/gedcom_media.go | Update media import to store notes as NoteList. |
| go-glx/gedcom_integration_test.go | Adapt integration tests to consume NoteList via .String(). |
| go-glx/gedcom_individual.go | Update individual import and census note attachment to append note entries. |
| go-glx/gedcom_import_test.go | Update import tests to expect NoteList rather than strings. |
| go-glx/gedcom_family.go | Update family import to store relationship notes as NoteList. |
| go-glx/gedcom_export.go | Update HEAD export to use NoteList for metadata notes. |
| go-glx/gedcom_export_test.go | Update export tests for NoteList-typed notes. |
| go-glx/gedcom_export_source.go | Update source export to use NoteList for NOTE emission. |
| go-glx/gedcom_export_roundtrip_test.go | Update roundtrip tests to expect NoteList in metadata. |
| go-glx/gedcom_export_repository.go | Update repository export to use NoteList for NOTE emission. |
| go-glx/gedcom_export_person.go | Update person/event/citation export note handling to use NoteList. |
| go-glx/gedcom_export_media.go | Update media export note handling to use NoteList. |
| go-glx/gedcom_export_family.go | Update family/relationship export note handling to use NoteList. |
| go-glx/gedcom_export_family_test.go | Update family export tests for NoteList notes. |
| go-glx/gedcom_evidence.go | Update citation/source note handling to append note entries. |
| go-glx/gedcom_converter.go | Update header conversion to append multiple notes into NoteList. |
| go-glx/gedcom_comprehensive_test.go | Update “has notes” checks to use IsEmpty(). |
| go-glx/diff_test.go | Update diff tests to use NoteList for notes comparisons. |
| go-glx/census.go | Wrap census-template string notes into NoteList for created entities/assertions. |
| go-glx/census_test.go | Update census tests to read notes via .String(). |
| glx/analyze_runner_test.go | Update analysis tests to use glxlib.NoteList. |
| glx/analyze_evidence.go | Update analysis logic to treat notes as NoteList (IsEmpty/String). |
Comments suppressed due to low confidence (4)
go-glx/gedcom_export_person.go:167
person.Notes.String()collapses a multi-noteNoteListinto a single GEDCOM NOTE record, which defeats preserving separate NOTE records. Consider iteratingperson.Notesand emitting one NOTE subrecord per entry (and only fall back toProperties["notes"]when the list is empty).
// NOTE - check both struct field and Properties map
noteText := person.Notes.String()
if noteText == "" {
if propNotes, ok := person.Properties[PropertyNotes].(string); ok {
noteText = propNotes
}
}
if noteText != "" {
record.SubRecords = append(record.SubRecords, &GEDCOMRecord{
Tag: GedcomTagNote,
Value: noteText,
})
}
go-glx/gedcom_export_person.go:461
event.Notes.String()collapses multiple notes into a single GEDCOM NOTE record. If NOTE record boundaries must be preserved on export, emit one NOTE subrecord per note inevent.Notes(and only use the Properties fallback when the list is empty).
// NOTE - check both struct field and Properties map
eventNoteText := event.Notes.String()
if eventNoteText == "" {
if propNotes, ok := event.Properties[PropertyNotes].(string); ok {
eventNoteText = propNotes
}
}
if eventNoteText != "" {
record.SubRecords = append(record.SubRecords, &GEDCOMRecord{
Tag: GedcomTagNote,
Value: eventNoteText,
})
}
go-glx/gedcom_export_family.go:404
event.Notes.String()collapses multi-note events into one GEDCOM NOTE record. If roundtrip fidelity requires preserving multiple NOTE records, emit one NOTE subrecord per entry inevent.Notes(and only fall back toProperties["notes"]when the list is empty).
// NOTE - check both struct field and Properties map
famEventNoteText := event.Notes.String()
if famEventNoteText == "" {
if propNotes, ok := event.Properties[PropertyNotes].(string); ok {
famEventNoteText = propNotes
}
}
if famEventNoteText != "" {
record.SubRecords = append(record.SubRecords, &GEDCOMRecord{
Tag: GedcomTagNote,
Value: famEventNoteText,
})
}
go-glx/gedcom_export_test.go:469
- Export tests updated here still only cover the single-note case. Since the PR’s goal is preserving separate GEDCOM NOTE records, add a test where
Noteshas 2+ entries and assert that the exported record contains multiple NOTE subrecords (one per note), rather than one NOTE with CONT lines.
repo := &Repository{
Name: "National Archives",
Address: "700 Pennsylvania Ave NW",
City: "Washington",
State: "DC",
PostalCode: "20408",
Country: "USA",
Website: "https://www.archives.gov",
Notes: NoteList{"Main facility"},
Properties: make(map[string]any),
}
record := exportRepository("repo-1", repo, expCtx)
assert.Equal(t, "@R1@", record.XRef)
assert.Equal(t, GedcomTagRepo, record.Tag)
// Check subrecords
var foundName, foundAddr, foundWWW, foundNote bool
for _, sub := range record.SubRecords {
switch sub.Tag {
case GedcomTagName:
foundName = true
assert.Equal(t, "National Archives", sub.Value)
case GedcomTagAddr:
foundAddr = true
// Check address subrecords
var hasCity, hasState, hasPost, hasCountry, hasAdr1 bool
for _, addrSub := range sub.SubRecords {
switch addrSub.Tag {
case GedcomTagAdr1:
hasAdr1 = true
assert.Equal(t, "700 Pennsylvania Ave NW", addrSub.Value)
case GedcomTagCity:
hasCity = true
assert.Equal(t, "Washington", addrSub.Value)
case GedcomTagStae:
hasState = true
assert.Equal(t, "DC", addrSub.Value)
case GedcomTagPost:
hasPost = true
assert.Equal(t, "20408", addrSub.Value)
case GedcomTagCtry:
hasCountry = true
assert.Equal(t, "USA", addrSub.Value)
}
}
assert.True(t, hasAdr1, "missing ADR1")
assert.True(t, hasCity, "missing CITY")
assert.True(t, hasState, "missing STAE")
assert.True(t, hasPost, "missing POST")
assert.True(t, hasCountry, "missing CTRY")
case GedcomTagWww:
foundWWW = true
assert.Equal(t, "https://www.archives.gov", sub.Value)
case GedcomTagNote:
foundNote = true
assert.Equal(t, "Main facility", sub.Value)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- All 8 export files now loop over NoteList and emit one GEDCOM NOTE subrecord per note, instead of collapsing with .String() - Fixed 3 stale "Combine notes" comments to "Collect notes" - NoteList.UnmarshalYAML explicitly sets nil for empty scalar nodes
|
All 10 Copilot items addressed in 7280f2c: Export fixes (6 threads) — All 8 export files now loop over NoteList and emit one GEDCOM NOTE subrecord per note instead of collapsing with .String(). Affected: gedcom_export.go (metadata), gedcom_export_person.go (person, event, citation), gedcom_export_source.go, gedcom_export_repository.go, gedcom_export_media.go, gedcom_export_family.go. Stale comments (3 threads) — "Combine notes" changed to "Collect notes" in gedcom_source.go, gedcom_repository.go, gedcom_media.go. UnmarshalYAML empty handling (1 thread) — Explicit |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- UnmarshalYAML: handle AliasNode by decoding the aliased node; treat empty values in the default branch as nil (consistent with empty scalar handling) to prevent synthetic empty notes. - exportFamilyEvent: emit one NOTE subrecord per NoteList element instead of collapsing via String(), matching the person/citation export pattern for roundtrip fidelity.
…gix/glx into fix/multi-note-roundtrip
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
YAML null scalars (null, ~) have node.Tag == "!!null" but non-empty
node.Value ("null", "~"), which would produce a note containing the
literal string. Now check for !!null tag and treat as nil.
Adds regression test for null/~/empty YAML inputs.
What and why
Multiple GEDCOM NOTE records on a person/family were concatenated into a single
Notes stringduring import. On export, they became one NOTE with CONT lines. Content was preserved but record boundaries were lost (queen: -7 notes, habsburg: -76 notes).The fix
New
NoteListtype ([]string) replacesNotes stringon all 10 entity types. Import appends separate notes instead of concatenating. Export emits each note as a separate GEDCOM NOTE record.NoteList type
Custom YAML marshal/unmarshal ensures backwards compatibility:
notes: "single string"unmarshals toNoteList{"single string"}(existing archives work)notes: ["first", "second"]unmarshals toNoteList{"first", "second"}(new multi-note)Scope
28 files changed, 295 insertions, 124 deletions. The
NoteListtype is 70 lines (including 10 tests). The rest is mechanical: string concatenation replaced withappend, string comparisons replaced with.IsEmpty(), string reads replaced with.String().Related issues
Fixes #487
Testing
Breaking changes
None for existing archives.
notes: "string"continues to work. The Go API changes fromstringtoNoteListbut the YAML representation is backwards compatible.