#429 : ensure that annotations / notes of kvps are written

[fbergmann]

Description

this pr syncs annotations before writing. That way potential cvterms are written out. I still would not recommend using kvps that way, but it is valid as far as SBML is concerned.

Motivation and Context

fixes #429

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Change in documentation

Checklist:

• I have updated all documentation necessary.
• I have checked spelling in (new) comments.

Testing

• Testing is done automatically and codecov shows test coverage
• This cannot be tested automatically

[Copilot]

Pull Request Overview

This PR ensures that annotations and notes in key-value pairs are properly synced before writing, addressing issue #429 by guaranteeing that potential cvterms are written out.

• Updated KeyValuePair::toXML() to sync annotations and add notes/annotations as children to the XML node.
• Extended test cases in TestWriteFbcExtension.cpp to verify that cvterm resources appear in the output XML.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/sbml/packages/fbc/sbml/KeyValuePair.cpp	Added syncing of annotations in toXML(), using const_cast for syncAnnotation.
src/sbml/packages/fbc/extension/test/TestWriteFbcExtension.cpp	Added test validations for checking that cvterm resources are written to XML.

Files not reviewed (1)

• src/sbml/packages/fbc/extension/test/test-data/fbc_example2_v3.xml: Language not supported

Comments suppressed due to low confidence (1)

src/sbml/packages/fbc/extension/test/TestWriteFbcExtension.cpp:435

• Please verify that ownership of the allocated CVTerm is correctly transferred via addCVTerm; if not, consider using a smart pointer or ensuring proper deletion to avoid memory leaks.

auto* term = new CVTerm();

[bgoli]

A general comment on the KVP annotations issue. Are they necessary, AFAIK KVP's don't inherit from SBase and I'm not at all sure if we want KVP's with annotations as it breaks the whole "easily to read" thing ... or am I just not seeing this properly?

[fbergmann]

They are not necessary as such. And indeed in the spec they dont inherit from SBase. In the libSBML implementation they did inherit from it though. And so the possibility in libSBML to set / get them was always there just not implemented.

Now someone found a use for them in the issue raised. Like i mentioned in the issue there, i would rather see the CVTerm on the SBase element itself rather than on the KVP.

[fbergmann]

@bgoli thinking about it further ... it is right that the KVP is not an sbase element as such, after all it lives in an annotation. That on the other hand means that anything goes, we dont enforce anything in annotations, as such there might as well be an annotation on the annotation. If anything we probably ought to add a best practice blurb into the spec, stating that it would be best to annotate the object as such, not one KVP.

[bgoli]

Thanks that clarifies things for me enough to move ahead.

[bgoli]

Thanks @fbergmann looks good to me.

[fbergmann]

this now also fixes #359