Skip to content

Merge existing Mappings#657

Merged
cmoesel merged 5 commits intomasterfrom
merge-existing-mappings
Nov 4, 2020
Merged

Merge existing Mappings#657
cmoesel merged 5 commits intomasterfrom
merge-existing-mappings

Conversation

@jafeltra
Copy link
Collaborator

@jafeltra jafeltra commented Oct 30, 2020

This PR adds support for adding rules to existing Mappings on StructureDefinitions (CIMPL-583).

On master, if you were to make a Mapping that has the same id as one that is already on the Parent of the Profile you are adding the mapping to, SUSHI will add a duplicate Mapping at the top level of the StructureDefinition and will also add any rules. This PR makes it so that you can add a Mapping that has the same id as one already on the SD and add rules. As long as any specified metadata matches what is on the existing Mapping, no metadata will be added to the SD but any rules on the Mapping will be added to the SD. If there is any metadata specified on the Mapping that does not match what is on the Mapping on the SD, an error will be logged and no rules will be added.

I had also intended to include fixes for the two things @cmoesel pointed out here, but after looking at the original US Core SDs, these changes seem to be caused by the caret value rules GoFSH was creating since we were accessing indexes in arrays in the differential. I believe all the mapping information should be the same as the original US Core SDs.

…n error if a mapping conflicts with one on the parent.
@jafeltra jafeltra force-pushed the merge-existing-mappings branch from 2e6f520 to 6a342d7 Compare October 30, 2020 21:28
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'm changing my mind on one thing we discussed. Let me know what you think!

@jafeltra
Copy link
Collaborator Author

jafeltra commented Nov 4, 2020

@cmoesel I've updated this to reflect the changes we talked about this morning. Hopefully the test description is a bit more clear, but all the new test descriptions are long, so I can try to reword them if they're still confusing.

27184ab makes it so that any inherited mapping that has additional or changed comments will add that field to the mapping on the structure definition. It also updates the test to check that the value on the mapping has been updated. I also added that check for the equality of the mapping for the first test to ensure nothing was changed when we don't update the metadata because there was no changed metadata. I can add that check to the other tests if we feel it is necessary.

29b5a6b just re-orders the tests so the test that checks for an error comes last in this group. The diff got confusing when I reordered it, so I just spit it into it's own commit so I could keep better track of the changes.

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.

It works! (No, I'm not actually surprised).

I ran GoFSH on US Core and then ran SUSHI master on the GoFSH results and this PR's branch on the GoFSH results. When I compared, there were differences: master created duplicate mappings in the SDs, but this PR's branch did not. So, that's GOOD!

Copy link
Contributor

@ngfreiter ngfreiter left a comment

Choose a reason for hiding this comment

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

Yep, this is looking good to me.

@cmoesel cmoesel merged commit 1c98676 into master Nov 4, 2020
@cmoesel cmoesel deleted the merge-existing-mappings branch November 4, 2020 19:04
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