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

Allow cardinality constraints to be applied to connected elements #1120

Merged
merged 4 commits into from
Jul 28, 2022

Conversation

mint-thompson
Copy link
Collaborator

Completes task CIMPL-986.

When constraining cardinality on an element, if there are connected elements that are the child of a slice, it may be possible to apply the cardinality constraint to those elements. As long as no cardinality is widened, applying the constraints is allowed. This may mean that only the new min or max are applied. If the new cardinality has no overlap with the connected element's existing cardinality, the cardinality constraint can't be applied.

When constraining cardinality on an element, if there are connected
elements that are the child of a slice, it may be possible to apply the
cardinality constraint to those elements. As long as no cardinality is
widened, applying the constraints is allowed. This may mean that only
the new min or max are applied. If the new cardinality has no overlap
with the connected element's existing cardinality, the cardinality
constraint can't be applied.
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

I think the general concept and approach is good, as it's friendlier than the spurious error we used to give and it's helpful in updating those cardinalities. When I ran regression, however, some of the changes in one repo didn't make sense to me.

You can reproduce by running regression on HL7/davinci-pas#master. You'll see a bunch of changes in StructureDefinition-profile-claim-update.json -- all of which constrain Claim.supportingInfo:SomeSliceName.extension and Claim.supportingInfo:SomeSliceName.modifierExtension to each have max 1. Here is one example:
image

If you look at the FSH definition for that profile, however, you'll see that the only rules on supportingInfo are these:

* supportingInfo.extension contains InfoChanged named infoChanged 0..1
* supportingInfo.extension[infoChanged] ^short = "A code indicating how the piece of information has changed."
* supportingInfo.modifierExtension contains InfoCancelledFlag named infoCancelledFlag 0..1
* supportingInfo.modifierExtension[infoCancelledFlag] ^short = "Indicates that this piece of information is not to be used."

It seems like adding an extension with max 1 (for that specific extension only) is going through and updating the max of the non-specific extension element in every slice. Which is not what it should be doing.

A connected element may end with a slice if the original element ends
with a slice with the same name.

When applying the "must support" flag on an element whose path ends with
a slice, the flag should be applied to connected elements only if the
connected element ends with a slice with the same name.
@jafeltra jafeltra self-assigned this Jul 26, 2022
jafeltra
jafeltra previously approved these changes Jul 27, 2022
Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

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

This makes sense to me! I have one incredibly small change in a comment, so I'm approving anyway.

test/export/StructureDefinitionExporter.test.ts Outdated Show resolved Hide resolved
@jafeltra
Copy link
Collaborator

Oh I also ran a full regression again, and there were no differences besides that one sliced element that has the MS flag properly applied (which was discussed in Slack).

cmoesel
cmoesel previously approved these changes Jul 28, 2022
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

This looks good and works well! Thanks, @mint-thompson. I'm approving this, but you probably should accept Julia's minor suggestion before we merge it.

@mint-thompson mint-thompson dismissed stale reviews from cmoesel and jafeltra via 209a995 July 28, 2022 18:55
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

Re-approval. Good work!

@cmoesel cmoesel merged commit fafc4b5 into master Jul 28, 2022
@cmoesel cmoesel deleted the cimpl-986-change-connected-cardinality branch July 28, 2022 19:36
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