Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add r3r4 templates #357

Merged
merged 18 commits into from
Apr 13, 2022
Merged

Add r3r4 templates #357

merged 18 commits into from
Apr 13, 2022

Conversation

sowu880
Copy link
Contributor

@sowu880 sowu880 commented Mar 7, 2022

No description provided.

@sowu880 sowu880 force-pushed the personal/sowu/add-R3R4-templates branch from 715bb1f to 35a5aef Compare March 10, 2022 16:02
@sowu880 sowu880 requested a review from irenepjoseph March 10, 2022 16:46
Copy link
Contributor

@irenepjoseph irenepjoseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed first round of templates

@BoyaWu10 BoyaWu10 mentioned this pull request Apr 8, 2022
@@ -12,6 +12,7 @@
},
{% endif -%}
],
"fhirVersion": "4.0.1",
Copy link
Contributor

@yankunhuang-pku yankunhuang-pku Apr 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious~ Do we need to update it in other resources?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, it may appear in ImplementationGuide:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be. In this first round of update, I only modify the value of "fhirVersion" field with 'N' priority, and plan to update others for template refinement. (Since value change in Stu3 to R4 is nice to have.)

@yankunhuang-pku
Copy link
Contributor

Just curious~ Do we have any tests to check the quality of templates now?

@@ -1,7 +1,7 @@
{% mergeDiff msg -%}
{
"targetProfile" : [
{{msg.profile | to_json_string}}
{{msg.profile | to_json_string | default : '""'}}
],
"profile" : "",
"binding" : {% include 'OperationDefinition/Binding' msg: msg.binding -%},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template name needs to be uppercase. OperationDefinition/parameter should be OperationDefinition/Parameter

"type" : {% include 'DataType/CodingToCodeableConcept' msg: msg.type -%},
"transform" : {% include 'DataType/ReferenceToCanonical' msg: msg.transform -%},
"dynamicValue" : [ {{ msg.dynamicValue | to_array | batch_render: 'PlanDefinition/DynamicValue', 'msg' }} ],
"action" : [ {{ msg.action | to_array | batch_render: 'PlanDefinition/action', 'msg' }} ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same~ PlanDefinition/action should be PlanDefinition/Action

"documentation" : [ {{ msg.documentation | to_array | batch_render: 'DataType/RelatedArtifact', 'msg' }} ],
"condition" : [ {{ msg.condition | to_array | batch_render: 'RequestGroup/Condition', 'msg' }} ],
"type" : {% include 'DataType/CodingToCodeableConcept' msg: msg.type -%},
"action" : [ {{ msg.action | to_array | batch_render: 'RequestGroup/action', 'msg' }} ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same~ RequestGroup/action should be RequestGroup/Action

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I checked all templates again to avoid lowercase. (I find that template name are case insensitive in local file system but sensitive in ACR templates. That's the reason why it not affect the result in my local tests.)
Case sensitive is not consistent in file system and ACR is a bug needs to fixed in the future. @qiwjin

@sowu880
Copy link
Contributor Author

sowu880 commented Apr 12, 2022

Just curious~ Do we have any tests to check the quality of templates now?

I have tests to enable convert results are validate R4 resource locally for all templates. But for now, we only have tests for 'N' priority stu3 resources in convert's unit tests. We can add more in the future and design more tests to check quality.

@sowu880 sowu880 merged commit d7291f8 into dotliquid Apr 13, 2022
@sowu880 sowu880 deleted the personal/sowu/add-R3R4-templates branch April 13, 2022 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants