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

Replace type.aggregation on ElementDefinition when modified #1433

Merged
merged 8 commits into from
Mar 29, 2024

Conversation

mint-thompson
Copy link
Collaborator

@mint-thompson mint-thompson commented Mar 5, 2024

Fixes #1411 and completes task CIMPL-1229.

If a rule modifies an element or child element of the type.aggregation array, any values on type.aggregation inherited from the parent are completely replaced. Properties of ElementDefinition with this behavior are defined in REPLACEMENT_PROPS.

No changes appeared in the regression output for this code change.

If a rule modifies an element or child element of the type.aggregation
array, any values on type.aggregation inherited from the parent are
completely replaced. Properties of ElementDefinition with this behavior
are defined in REPLACEMENT_PROPS.
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.

Wow, I didn't realize it was so tricky to clear type.aggregation! The tests were super helpful to see this working. I asked two small questions that were mostly for my own understanding of the code.

src/fhirtypes/ElementDefinition.ts Show resolved Hide resolved
src/fhirtypes/ElementDefinition.ts Outdated Show resolved Hide resolved
jafeltra
jafeltra previously approved these changes Mar 23, 2024
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.

I like that we're able to avoid re-calling the code to clear elements now, and the process to clear looks about as straightforward as possible. Looks good to me.

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. I think the recent improvements really help a lot.

I tested it with this simple (and technically incorrect) case:

Profile: MyPatient1
Parent: Patient
* name ^type.aggregation[0] = #contained
* name ^type.aggregation[+] = #referenced

Profile: MyPatient2
Parent: MyPatient1
* name ^type.aggregation = #contained

I suggested a few comments to help future me (or future someone else) as they maintain the code. That said, if you think it's not necessary and the comments will just add clutter, then feel free to push back.

src/fhirtypes/ElementDefinition.ts Outdated Show resolved Hide resolved
src/fhirtypes/ElementDefinition.ts Show resolved Hide resolved
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.

Still looks good to me.

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.

Excellent. Thank you for working through all our feedback on this. It looks great!

@mint-thompson mint-thompson merged commit 0199cbc into master Mar 29, 2024
14 checks passed
@mint-thompson mint-thompson deleted the cimpl-1229-replace-aggregation branch March 29, 2024 13:00
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.

SUSHI should replace ElementDefinition.type.aggregation array when modified in FSH
3 participants