Fix controlled term base activation for schema versions#89
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "contactInformation", contacts(person.email) }; | ||
|
|
||
| if ommtest.oneoffs.currentSchemaMajorVersion() >= 5 | ||
| personArguments = [personArguments, { ... |
Test Results (R2021b)700 tests +5 692 ✅ +2 2m 4s ⏱️ -7s For more details on these errors, see this check. Results for commit f23a74e. ± Comparison against base commit 674e240. ♻️ This comment has been updated with latest results. |
Test Results (R2024b)700 tests +5 695 ✅ +5 2m 3s ⏱️ +14s For more details on these errors, see this check. Results for commit f23a74e. ± Comparison against base commit 674e240. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR fixes controlled-term base-class activation across openMINDS schema versions by introducing versioned controlled-term base templates and activating the correct one during model version selection, with accompanying unit tests.
Changes:
- Add a shared
ControlledTermBaseand split version-specificControlledTermbase templates (v2/v3) under a private folder. - Activate the correct controlled-term base during
selectOpenMindsVersion, and clear MATLAB class caches on version switches. - Add unit tests covering controlled-term construction and version-specific property presence/absence.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/tests/unitTests/ControlledTermTest.m | Adds unit coverage for controlled-term construction and property sets. |
| code/internal/+openminds/selectOpenMindsVersion.m | Activates controlled-term base on version selection; clears cached class definitions. |
| code/internal/+openminds/+internal/activateControlledTermBase.m | Implements controlled-term base activation by copying the correct versioned template. |
| code/internal/+openminds/+abstract/private/controlledTerms/v3/ControlledTerm.m | Adds v3 controlled-term base template (newer cross-reference properties). |
| code/internal/+openminds/+abstract/private/controlledTerms/v2/ControlledTerm.m | Adds v2 controlled-term base template (older InterLex/KnowledgeSpace properties). |
| code/internal/+openminds/+abstract/ControlledTermBase.m | Introduces shared controlled-term initialization/deserialization behavior. |
| code/internal/+openminds/+abstract/ControlledTerm.m | Refactors ControlledTerm to inherit shared behavior from ControlledTermBase. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if startsWith(instanceSpec, openminds.constant.BaseURI) | ||
| obj.deserializeFromName(instanceSpec); | ||
| elseif isfile( instanceSpec ) | ||
| obj.load( instanceSpec ) % todo: Not implemented?? | ||
| else | ||
| % Deserialize from name of controlled instance | ||
| obj.deserializeFromName(instanceSpec); | ||
| end |
There was a problem hiding this comment.
initializeControlledTerm calls obj.load(instanceSpec) when instanceSpec is a local file, but there is no load method defined on openminds.abstract.Schema (or elsewhere for controlled terms). Passing a filename will therefore throw a runtime error despite the API implying it is supported. Either implement a controlled-term file loading path here (e.g., deserialize from JSON-LD) or remove the isfile branch and update the accepted-input logic accordingly.
| function controlledTermVersion = activateControlledTermBase(modelVersion) | ||
| %activateControlledTermBase Activate the controlled-term base for a model version. | ||
|
|
||
| arguments | ||
| modelVersion (1,1) openminds.internal.utility.VersionNumber ... | ||
| {openminds.mustBeValidVersion(modelVersion)} = "latest" | ||
| end | ||
|
|
||
| controlledTermVersion = getControlledTermVersion(modelVersion); |
There was a problem hiding this comment.
There’s no unit test asserting that switching the openMINDS model version in the same MATLAB session actually swaps the active openminds.abstract.ControlledTerm definition (e.g., openminds.version(4) exposes interlexIdentifier/knowledgeSpaceLink, then switching back to latest exposes otherCrossReference/preferredCrossReference). Adding such a test would protect the version-switching + clear classes behavior from regressions.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| % Version selection can replace shared abstract class files. | ||
| % Clear cached class definitions so MATLAB sees the active files. | ||
| clear classes; |
Summary
Verification
/Applications/MATLAB_R2025b.app/bin/matlab -batch "addpath(fullfile(pwd,'code')); startup('latest'); addpath(genpath(fullfile(pwd,'tools','tests'))); results = runtests('tools/tests/unitTests/ControlledTermTest.m'); assertSuccess(results);"Stacked on #90. Note:
AGENTS.mdwas intentionally not included in this PR.