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

2555 Add support for multiple valuesets and system filters to ValueSetExpander #2558

Merged
merged 4 commits into from
Aug 3, 2023

Conversation

ewoutkramer
Copy link
Member

@ewoutkramer ewoutkramer commented Aug 2, 2023

Expander now supports having more than one valueset in an include and specifying a valueset + system to limit the import of said valuesets.

Also reworked the expander to more closely follow the expansion algorithm specified in the spec.

  • Using more delayed processing, so processing valuesets that are too big is actually stopped before attempting it.
  • Documented all steps more clearly, referring to the spec.
  • specifying both System and ValueSet will now do an intersection.
  • Cyclic references between composes are detected

# Conflicts:
#	src/Hl7.Fhir.STU3/Specification/Terminology/ValueSetExpander.cs
#	src/Hl7.Fhir.Specification.STU3.Tests/Source/TerminologyTests.cs
…specified in the spec.

- Using more delayed processing, so processing valuesets that are too big is actually stopped before attempting it.
- Documented all steps more clearly, referring to the spec.
- specifying both System and ValueSet will now do an intersection.
@ewoutkramer
Copy link
Member Author

Note: this PR only contains the changes for STU3, for ease of review.

PR #2559 will make sure this functionality also exists for R4+, but doing that in this PR would make reviewing hard due to reorganizations in the directory structure.

TL;DR: first review this PR, then 2559 and if both are ok, merge this one first, then 2559.

@ewoutkramer ewoutkramer marked this pull request as ready for review August 2, 2023 14:56
{
if (inclusionChain.Contains(uri))
throw new TerminologyServiceException($"ValueSet expansion encountered a cycling dependency from {inclusionChain.Peek()} back to {uri}.");
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I was wondering how you would solve this, nice

@mmsmits mmsmits enabled auto-merge August 3, 2023 14:57
@mmsmits mmsmits self-requested a review August 3, 2023 14:58
@mmsmits mmsmits merged commit f0ad594 into develop Aug 3, 2023
15 checks passed
@mmsmits mmsmits deleted the feature/expand-ts-capabilities branch August 3, 2023 14:59
@mmsmits
Copy link
Member

mmsmits commented Aug 4, 2023

fixes #2555

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.

Add support for multiple valuesets and system filters to the ValueSetExpander
2 participants