-
Notifications
You must be signed in to change notification settings - Fork 24
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 race and ethnicity bindings to bindings collection #531
Conversation
…443-race-ethnicity-to-spreadsheet
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #531 +/- ##
============================================
- Coverage 21.91% 21.85% -0.06%
- Complexity 1672 1673 +1
============================================
Files 297 297
Lines 25317 25390 +73
Branches 3990 4005 +15
============================================
+ Hits 5548 5550 +2
- Misses 18848 18920 +72
+ Partials 921 920 -1 ☔ View full report in Codecov by Sentry. |
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.
I am not able to test anything in this PR, so I am unable to verify functionality. A few small changes requested and some comments.
Nice work!
@@ -121,10 +121,84 @@ private void getBindings(String sdName, List<ElementDefinition> eds, String sdUR | |||
sdbo.setBindingValueSetVersion(valueSetVersion); | |||
bindingObjects.put(sdName + "." + sdbo.getElementId(), sdbo); | |||
} | |||
else if(ed.getId().equalsIgnoreCase("patient.extension:race") |
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.
I may be missing some context, but is this really what we want here? Seems like we want to generalize this for all potential extensions with bindings, right? Something like ed.hasExtension()
or type checking on the ElementDefinition.
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.
Applied
} | ||
sdbo.setBindingValueSetVersion(valueSetVersion); | ||
if (null != elementValueSet) { | ||
StringBuilder codeSystemURLs = new StringBuilder();; |
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.
Extra ";"
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.
Applied
StringBuilder codeSystemURLs = new StringBuilder();; | ||
Map<String, String> codeSystemsMap = new HashMap<>(); | ||
getValueSetCodeSystems(elementValueSet, codeSystemsMap); | ||
if(null != codeSystemsMap && !codeSystemsMap.isEmpty()) { |
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.
codeSystemsMap will never be null (see line 181)
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.
Applied
valueSetVersion = this.canonicalResourceDependenciesAtlas.getValueSets().getByCanonicalUrlWithVersion(sdbo.getBindingValueSetURL()).getVersion(); | ||
sdbo.setBindingValueSetName(this.canonicalResourceDependenciesAtlas.getValueSets().getByCanonicalUrlWithVersion(sdbo.getBindingValueSetURL()).getName()); | ||
elementValueSet = this.canonicalResourceDependenciesAtlas.getValueSets().getByCanonicalUrlWithVersion(sdbo.getBindingValueSetURL()); | ||
} else if (valueSetVersion.length() < 1 && bindingValueSet.contains("|")) { |
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.
Is checking the valueSetVersion string length necessary here? Seems like it is set in the previous if statements. If it is necessary, use valueSetVersion.isEmpty()
.
CanonicalType canonicalType = new CanonicalType(); | ||
canonicalType.setValue(String.valueOf(ed.getType().get(0).getProfile().get(0))); | ||
ed.getType().get(0).getProfile().get(0).getValueAsString(); | ||
String urlValue = ed.getType().get(0).getProfile().get(0).toString(); |
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.
This is set, but never 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.
Applied
tooling/pom.xml
Outdated
@@ -333,6 +333,12 @@ | |||
<artifactId>mockserver-client-java</artifactId> | |||
<version>5.14.0</version> | |||
</dependency> | |||
<dependency> |
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.
I didn't notice where this dependency is being used in this PR.
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.
Applied
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! Waiting on @brynrhodes to give the final go-ahead.
Issue #433 QI Core Value Set and Code System Query Issue
Description
visitExtensions was added to StructureDefinitionElementBindingVisitor to visit the extensions and collect the bindings especially for patient.extension:race and patient.extension:ethnicity.
By creating this PR you acknowledge that your contribution will be licensed under Apache 2.0