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

Extract extension contexts to keyword #244

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

mint-thompson
Copy link
Collaborator

Completes #234 and task CIMPL-1163.

When processing an Extension definition, set the contexts directly on the ExportableExtension object. When doing so, convert paths back to FSH-style paths. When extracting caret value rules on an Extension definition, do not extract any rules for contexts.

Add optimizer plugin to resolve URLs in contexts.

This ended up being a bit simpler than I had anticipated in regards to differentiating between core and non-core resources. Since SUSHI does the hard work of actually resolving contexts, GoFSH gets to use the name of anything that it can fish up, no matter where it came from.

When processing an Extension definition, set the contexts directly on
the ExportableExtension object. When doing so, convert paths back to
FSH-style paths. When extracting caret value rules on an Extension
definition, do not extract any rules for contexts.

Add optimizer plugin to resolve URLs in contexts.
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.

The code here looks really good. I left one minor comment about test descriptions inline.

I ran this branch on US Core, which led me to two questions:

  1. This came up when I compared the round trip results from US Core and I saw differences that I wasn't expecting. However, this might actually be an issue with SUSHI, and not this branch. It came up in USCoreEthnicityExtension, USCoreRaceExtension, and USCoreTribalAffiliationExtension. They all have a context like this:
{
  "type": "element",
  "expression": "FamilyMemberHistory"
}

This branch creates a Context: FamilyMemberHistory in the FSH, but then when SUSHI outputs that context into JSON, it creates a context like this:

{
  "expression": "http://hl7.org/fhir/StructureDefinition/DiagnosticReport-geneticsFamilyMemberHistory",
  "type": "extension"
}

Assuming the Context keyword looks correct, maybe this is just a SUSHI issue.

  1. I wanted to double check that this was expected since there didn't seem to be a caret value rule created for the context before this branch, but USCoreJurisdictionExtension has a context array this like this:
"context": [
  {
    "type": "element",
    "expression": "Element"
  }
]

The released version of GoFSH didn't create any caret rules for this context, but this branch does create Context: Element. I think that Context keyword is correct, but the missing caret value rule made me not sure, so I wanted to check.

src/optimizer/utils.ts Show resolved Hide resolved
test/optimizer/plugins/ResolveContextURLsOptimizer.test.ts Outdated Show resolved Hide resolved
@mint-thompson
Copy link
Collaborator Author

@jafeltra Thank you for the review!

  1. That change in USCore is a SUSHI bug, similar to the one recently found for Instances of Identifier: if you look at StructureDefinitionExporter.setContext, you'll see that it fishes for extensions before any other type.
  2. It looks like that's the default context that SUSHI sets when no Context keyword is used. There's a corresponding RemoveDefaultExtensionContextRulesOptimizer in GoFSH. This optimizer probably doesn't do much of anything anymore. So, the optimizer should change to instead check the context on each extension, and if the only context is the default, remove it so that no Context keyword is used.

The optimizer now checks the extension's contexts array, rather than
caret value rules.

Fix some test descriptions for ResolveContextURLsOptimizer.
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.

Looks good to me! Thanks for updating that optimizer and answering my questions!

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 is fantastic, and quite clean. I like the way it turned out!

Copy link
Member

Choose a reason for hiding this comment

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

Ooooh. This optimizer is way nicer now!

@mint-thompson mint-thompson merged commit da2d301 into master Oct 11, 2023
14 checks passed
@jafeltra jafeltra deleted the cimpl-1163-context-keyword branch January 11, 2024 22:41
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