add missing AOM 2 fields and types #2
Conversation
There was a problem hiding this comment.
Hi @pieterbos, can you please review my suggestions and submit it again?
Thanks!
components/AM/latest/Archetype.xsd
Outdated
| @@ -248,10 +311,18 @@ | |||
| </xs:complexType> | |||
| <xs:complexType name="ARCHETYPE_TERMINOLOGY"> | |||
| <xs:sequence> | |||
| <!-- possible here, but redundant: is_differential, original_language, concept_code. add? --> | |||
There was a problem hiding this comment.
I would add them as optional
components/AM/latest/Archetype.xsd
Outdated
| <xs:element name="terminology_extracts" type="CodeDefinitionSet" minOccurs="0" maxOccurs="unbounded"/> | ||
| </xs:sequence> | ||
| </xs:complexType> | ||
| <xs:complexType name="INCLUDED_TERMINOLOGY"> |
There was a problem hiding this comment.
Where is this originating from? I cannot find the specs for this; I assume is related to OPERATIONAL_TEMPLATE, but so far there I see no specs clearly specifying it.
If this is not used yet (so this is a new class introduced by this change), I suggest to name it ArchetypeTerminologyItem to be in line with other similarly defined classes.
| <xs:complexType name="INCLUDED_TERMINOLOGY"> | |
| <xs:complexType name="ArchetypeTerminologyItem"> | |
| <xs:complexContent> | |
| <xs:extension base="ARCHETYPE_TERMINOLOGY"> | |
| <xs:attribute name="id" type="xs:string" use="required"/> | |
| </xs:extension> | |
| </xs:complexContent> | |
| </xs:complexType> |
There was a problem hiding this comment.
Operational template has the component_terminogies field/element. that is where this is used. It is defined in https://specifications.openehr.org/releases/AM/latest/AOM2.html#_operational_template_class as:
Compendium of flattened terminologies of archetypes externally referenced from this archetype, keyed by archetype identifier. This will almost always be present in a[n operational] template.
So it is literally a complete included terminology, plus the archetype id where it is from. That's why I called it INCLUDED_TERMINOLOGY, but indeed this is just an XML complexType, not an OpenEHR type. I could rename it. Given this information, do you think it should be IncludedTerminology or ArchetypeTerminologyItem?
There was a problem hiding this comment.
My reasoning was that this class (type) is also used at OPERATIONAL_TEMPLATE.terminology_extracts, therefore has to be named differently than 'includedTerminology'. It should also be descriptive, i.e. should indicate that this is a type used as one xml-item of the Hash<String,ARCHETYPE_TERMINOLOGY>.
Perhaps @wolandscat have a better suggestion?
There was a problem hiding this comment.
So you are proposing two things:
- extend ARCHETYPE_TERMINOLOGY instead of including it as a parameter.
- find a better name
extending: sounds good, I'll change that.
The name:
This really is another terminology, or an extract of it which itself is a terminology as well, included in this archetype. Both for the component_terminologies and terminology_extracts. So I think INCLUDED_TERMINOLOGY is a fitting name. It could be IncludedTerminology as well of course.
It also is an archetype terminology keyed by its id-field. That could be: ArchetypeTerminologyDictionaryItem , just like StringDictionaryItem. Perhaps we need a native speaker. @wolandscat ?
Actually the terminology_extracts attribute on OPERATIONAL_TEMPLATE does not make sense to me at all on the operational template level - it is keyed by value/at-code, but an at-code in an OPT is meaningless without its path, since it can come from one of many included archetypes. I would say the whole attribute needs to be removed, and the already existing terminology_extracts in the component_terminologies should be used instead. But that is a different issue, I'll create a problem report.
There was a problem hiding this comment.
On reading the above.... I'd probably go for IncludedTerminology, so it doesn't make it look like there is an AOM type of that name (there isn't). But 'included terminology' is more informative than 'ArchetypeTerminologyItem' which doesn't tell you anything much, even if it is somehow more technically 'right'.
I don't tend to get too religious about what appears in an XSD, except for one thing - it's good practice to make it clear which classes are direct reprsentations of a class in the upstream specs (RM, AM etc) and which ones are not (can be quite a few helper classes in an XSD). I also think it's better to make the XSDs as intelligible as possible in a standalone way for implementers who are not going to go look in the upstream specs. Just my view ;)
There was a problem hiding this comment.
Changed. There are many more issues probably in these files. I'll generate a schema from archie, and see how that looks.
components/AM/latest/Archetype.xsd
Outdated
| <xs:complexContent> | ||
| <xs:extension base="AUTHORED_ARCHETYPE"> | ||
| <xs:sequence> | ||
| <xs:element name="template_overlays" type="TEMPLATE_OVERLAY" nillable="true" minOccurs="0" maxOccurs="unbounded"/> |
There was a problem hiding this comment.
is @nillable necessary here (and also on next OPERATIONAL_TEMPLATE)?
There was a problem hiding this comment.
Oh you're right. No, it shouldn't be allowed, I'll change it soon.
Co-Authored-By: pieterbos <[email protected]>
Note that this pull request is not ready, I need to validate it and to check more with the specifications and our implementation. However, I think it's a good point in time to discuss whether these should be the changes to the AOM XSD, and wether we should change anything first.
The temporal fields still need some work, looks like they don't have a proper constraint, or at least nog all of them.