Skip to content

Remove all non-inheritable extensions from the base profile before snapshotting - #2886

Merged
mmsmits merged 14 commits into
developfrom
feature/1814-remove-all-non-inheritable-extensions-from-base
Oct 23, 2024
Merged

Remove all non-inheritable extensions from the base profile before snapshotting#2886
mmsmits merged 14 commits into
developfrom
feature/1814-remove-all-non-inheritable-extensions-from-base

Conversation

@mmsmits

@mmsmits mmsmits commented Oct 8, 2024

Copy link
Copy Markdown
Member

fixes #1814

@mmsmits
mmsmits marked this pull request as ready for review October 8, 2024 09:34
@mmsmits
mmsmits requested a review from ewoutkramer October 8, 2024 09:35
@mmsmits
mmsmits enabled auto-merge October 8, 2024 09:35
// Also ignore any Changed extensions on base and diff
elemClone.RemoveAllConstrainedByDiffExtensions();
baseClone.RemoveAllConstrainedByDiffExtensions();
elemClone.RemoveAllNonInheritableExtensions();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this method replace the original ones? So RemoveallNonInheritableExtension replaces RemoveAllConstrainedByDiffExtensions? If so, should we not remove/obsolete the original method?

@mmsmits mmsmits Oct 21, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, I thought about that, but didn't dare to since Forge might be using the ConstraintByDiff extension one maybe?
And that is a bit faster (I don't know if it might be noticeable in a UI).
@Rob5045 any feedback here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Forge does not use the RemoveAllConstrainedByDiffExtensions extension.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll Obsolete is then

@Rob5045 Rob5045 Oct 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm working on supporting extensions on element definition properties in Forge. That would mean that the UI won't be cluttered with these I guess? That would be nice.
image

@Rob5045 Rob5045 Oct 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about http://hl7.org/fhir/StructureDefinition/elementdefinition-bindingName? I see that one all the time as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@mmsmits
mmsmits requested a review from ewoutkramer October 22, 2024 13:57
Comment thread src/Hl7.Fhir.Base/Specification/Snapshot/SnapshotGeneratorExtensions.cs Outdated
Comment thread src/Hl7.Fhir.Base/Specification/Snapshot/SnapshotGeneratorExtensions.cs Outdated
mmsmits and others added 3 commits October 22, 2024 16:57
@mmsmits
mmsmits requested a review from ewoutkramer October 22, 2024 14:58
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.

Make sure we do not propagate "publishing extensions" while creating a snapshot

3 participants