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

Construct inline instances from within more FSH entity types #270

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

mint-thompson
Copy link
Collaborator

Description:
Logical models, Resources, CodeSystems, and ValueSets are now checked for rules that could be used to define an inline instance. Since this means those entities will now contain rules that assign inline instances, check those entity types when simplifying instance names.

Testing Instructions:
Confirm that the new test cases demonstrate that inline instances are created for rules contained on the newly added FSH entity types. Confirm that the sample JSON in the related issue now results in an inline instance of CodeSystem. Note that a round trip will not be successful with the sample JSON, since #example-codesystem is not a value that should be used for referring to a contained CodeSystem. The correct output for this JSON is shown in FHIR/sushi#1520.

Related Issue:
Fixes #252

Logical models, Resources, CodeSystems, and ValueSets are now checked
for rules that could be used to define an inline instance. Since this
means those entities will now contain rules that assign inline
instances, check those entity types when simplifying instance names.
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 looks pretty good to me. Those comments pointing to the two optimizers were very helpful!

I just have one question for my own understanding about what you said in the PR description about that third item on the issue:

Note that a round trip will not be successful with the sample JSON, since #example-codesystem is not a value that should be used for referring to a contained CodeSystem.

Do you mean that right now #example-codesystem is not a valid value to be used to refer to a contained CodeSystem? We do still have FHIR/sushi#1403 to support that, so should we create an optimizer for GoFSH for that extension pattern once that syntax is supported in SUSHI?

I think any answer to that question doesn't affect this PR, so I'm going to approve in the meantime!

@mint-thompson
Copy link
Collaborator Author

Do you mean that right now #example-codesystem is not a valid value to be used to refer to a contained CodeSystem? We do still have FHIR/sushi#1403 to support that, so should we create an optimizer for GoFSH for that extension pattern once that syntax is supported in SUSHI?

Yes, that's correct. The JSON in the linked issue is not correct according, since it should be using the contained CodeSystem's url as the value of system, and should also contain the valueset-system extension as seen in FHIR/sushi#1520. With that JSON, the fshing trip is successful.

As a related issue: right now, the generated FSH for the correct ValueSet will be an Instance, not a ValueSet. This is because the presence of the valueset-system extension looks like something that would need to be manually assigned. But, since we know SUSHI will put that in there when we need it, we don't need rules for it as long as we can turn it into ValueSet definition. Do you think making that change should be part of this PR, or should it be a separate issue?

@mint-thompson
Copy link
Collaborator Author

As a related issue: right now, the generated FSH for the correct ValueSet will be an Instance, not a ValueSet. This is because the presence of the valueset-system extension looks like something that would need to be manually assigned. But, since we know SUSHI will put that in there when we need it, we don't need rules for it as long as we can turn it into ValueSet definition. Do you think making that change should be part of this PR, or should it be a separate issue?

After some discussion with the team, I think it's fair to say that this optimization is a separate issue. I've created #271 to track this.

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 to me as well. Thank you, @mint-thompson!

@mint-thompson mint-thompson merged commit c10b13b into master Oct 18, 2024
14 checks passed
@mint-thompson mint-thompson deleted the construct-more-inline-instances branch October 18, 2024 14: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.

GoFSH handles inline CodeSystems poorly
3 participants