Conversation
| # assumption: 1 class per dataset - many need to expand this for ADaM | ||
| if dataset.get("observationClass", {}).get("name", ""): | ||
| ds_class = dataset.get("observationClass", {}).get("name", "").upper().replace("-", " ") | ||
| ds_class = dataset["observationClass"]["name"].upper().replace("-", " ") |
There was a problem hiding this comment.
Do you want to create the code to address subclasses or should we wait until we get to ADaM?
There was a problem hiding this comment.
If the content is available in the DDS, then yes, I'll add it in the next update.
| :param attr: ItemRef template attributes to update with optional values | ||
| :param obj: variable definition dictionary from the DDS JSON | ||
| """ | ||
| # TODO does method exist on items in the DDS? |
There was a problem hiding this comment.
Methods don't exist in DDS though some of them (i.e. BMI) have a origin type set to Derived, which triggers an issue in P21 as it expects a method.
There was a problem hiding this comment.
I added Methods to cases with an origin type of Derived. I added the MethodOID to the ItemRef and then generated a MethodDef. The BMI example looks like this:
<MethodDef OID="MT.VS.VSORRES.BMI" Name="Derive VSORRES BMI" Type="Computation"> <Description> <TranslatedText xml:lang="en">__PLACEHOLDER__ for derivation of VSORRES BMI</TranslatedText> </Description> </MethodDef>
| "Originator": "360i Define-XML Team", | ||
| "SourceSystem": "odmlib", | ||
| "SourceSystemVersion": "0.2", | ||
| "Context": "Other", |
There was a problem hiding this comment.
Context could also have as a value "Submission". Where should this information come from?
There was a problem hiding this comment.
My original thinking was that we were generating a Define-XML Spec, so it would be Other. Now I think it would be good to provide the Context as input, possibly with a default value of Other. It's important because having a Submission context triggers additional conformance rules. This could be added as a command-line argument/
| ir.WhereClauseRef.append(wc) | ||
| applicable_when = item.get("applicableWhen") or [] | ||
| if applicable_when: | ||
| # TODO when there are multiple applicableWhen values only the first is wired up |
There was a problem hiding this comment.
Does that mean the "OR" statement is not supported?
There was a problem hiding this comment.
Yes, that's what it means. If DDS supports it, I'll add it in the next update.
I'll go through all the remaining TODOs and address as many as I can.
dostiep
left a comment
dostiep
left a comment
There was a problem hiding this comment.
I've made a few comments, nothing major, just questions I have mostly. Otherwise, it looks pretty good.
2 participants
Here are fixes for the P21 Community issues I could resolve. This is Issue #71. I added some comments in the P21 issue report that I will share via Slack. I also fixed a few other small issues.