-
Notifications
You must be signed in to change notification settings - Fork 49
[issue-58] adding identical ChoiceType support and tests, upgrading to… #59
Conversation
@@ -144,12 +144,16 @@ | |||
<argument>http://hl7.org/fhir/StructureDefinition/AllergyIntolerance</argument> | |||
<argument>http://hl7.org/fhir/StructureDefinition/Procedure</argument> | |||
|
|||
<argument>http://hl7.org/fhir/StructureDefinition/Medication</argument> | |||
<argument>http://hl7.org/fhir/StructureDefinition/MedicationDispense</argument> | |||
<argument>http://hl7.org/fhir/StructureDefinition/MedicationRequest</argument> | |||
<argument>http://hl7.org/fhir/StructureDefinition/MedicationStatement</argument> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also include few more resources down below ?
<argument>http://hl7.org/fhir/StructureDefinition/ProcedureRequest</argument> <argument>http://hl7.org/fhir/StructureDefinition/Encounter</argument> <argument>http://hl7.org/fhir/StructureDefinition/CarePlan</argument> <argument>http://hl7.org/fhir/StructureDefinition/Claim</argument> <argument>http://hl7.org/fhir/StructureDefinition/Immunization</argument> <argument>http://hl7.org/fhir/StructureDefinition/EpisodeOfCare</argument> <argument>http://hl7.org/fhir/StructureDefinition/Measure</argument> <argument>http://hl7.org/fhir/StructureDefinition/MeasureReport</argument>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added these resources, 44b18b5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Just 1 housekeeping comment.
@@ -405,7 +406,10 @@ public HapiFieldSetter toHapiConverter(BaseRuntimeElementDefinition... elementDe | |||
|
|||
private String recordNameFor(String elementPath) { | |||
|
|||
return elementPath.replaceAll("\\.", ""); | |||
return Arrays.stream(elementPath.split("\\.")) | |||
.map(StringUtils::capitalize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a pre-existing custom implementation of capitalize down below: https://github.com/cerner/bunsen/pull/59/files#diff-59b60ed682a9e8e75a39b15530d028d9R605
Probably should only keep 1 implementation in use. I suggest removing the custom implementation and instead use the StringUtils implementation in the 1 place below where that is currently being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the custom implementation in favor of the StringUtils
library, 44b18b5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM. Thanks @bdrillard for addressing this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM. Thanks for the changes!
Merged! Thanks for the review, all! |
… Spark 2.4.4
Summary
Closes #58, adding support for Avro Schema generation over FHIR Resources that have fields with identical ChoiceType structures.
Additional Details
Also bumps the 0.5.0-dev branch to Spark 2.4.4.