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

Issue #2163 - throw exception when filter cannot be applied / created #2172

Merged
merged 4 commits into from
Mar 30, 2021

Conversation

JohnTimm
Copy link
Collaborator

@JohnTimm JohnTimm commented Mar 30, 2021

Signed-off-by: John T.E. Timm johntimm@us.ibm.com

  1. throw FHIRTermException from CodeSystemSupport when concept filter cannot be created
  2. throw FHIRTermServiceException from GraphTermServiceProvider and RegistryTermServiceProvider when filter cannot be applied or in response to FHIRTermException being thrown from CodeSystemSupport
  3. convert is-a, generalizes and is-not-a into in and not-in when code system hierarchy is undefined
  4. create common set of tests in FHIRTermServiceProviderTest base class
  5. rework CodeSystemSupport and RegistryTermServiceProvider to properly support normalization of code values and match GraphTermServiceProvider implementation
  6. move toObject and toLong methods from FHIRTermGraphUtil to CodeSystemSupport
  7. create toSet(CodeSystem, Collection<Concept>) and toString(Concept) utility methods to be reused in various places across CodeSystemSupport

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
Copy link
Contributor

@prb112 prb112 left a comment

Choose a reason for hiding this comment

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

LGTM. small comments. It does make me think that we need a CICD solution for this.

Comment on lines +39 to +41
return concept.toBuilder()
.concept(Collections.emptyList())
.build();
Copy link
Member

Choose a reason for hiding this comment

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

why do we empty the concept list before returning it?

Copy link
Collaborator Author

@JohnTimm JohnTimm Mar 30, 2021

Choose a reason for hiding this comment

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

We are flattening / removing the inherent concept hierarchy so that it more closely models what GraphTermServiceProvider returns. Consumers of these concepts care about code, display, designations and properties (not the inherent hierarchy).

Copy link
Member

@lmsurpre lmsurpre Mar 30, 2021

Choose a reason for hiding this comment

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

it might be worth adding a comment to explain that. maybe even to the parent javadoc to explain the contract for users of the FHIRTermServiceProvider interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

Asked John offline to address in upcoming PR

Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
Signed-off-by: John T.E. Timm <johntimm@us.ibm.com>
Copy link
Member

@lmsurpre lmsurpre left a comment

Choose a reason for hiding this comment

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

LGTM

@prb112 prb112 merged commit b97fb7f into main Mar 30, 2021
@prb112 prb112 deleted the johntimm-main branch March 30, 2021 13:46
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.

3 participants