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

Allow name/description in Instances #1104

Merged
merged 11 commits into from
Jul 20, 2022
Merged

Conversation

guhanthuran
Copy link
Contributor

Fixes #1003

Allows users to put a specified Title and/or Description in their instances.
To test, use this: https://fshschool.org/FSHOnline/#/share/32OCOPB

If the Instance has the title and description filled out then it is working.

Copy link
Collaborator

@mint-thompson mint-thompson left a comment

Choose a reason for hiding this comment

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

There is one other place where _instanceMeta.title and _instanceMeta.description are checked: in IGExporter.addResources, these values can be used when populating the resource list. So, that function should be updated to look for title and description directly on the InstanceDefinition.

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 a bit too agressive in setting title and description. See individual comments for details.

src/export/InstanceExporter.ts Outdated Show resolved Hide resolved
src/export/InstanceExporter.ts Outdated Show resolved Hide resolved
test/export/InstanceExporter.test.ts Outdated Show resolved Hide resolved
@cmoesel cmoesel self-assigned this Jun 21, 2022
@cmoesel
Copy link
Member

cmoesel commented Jun 28, 2022

I will run regression on this to see how many changes it introduces, but I'm thinking we might want to merge this into next instead of master -- so it goes into the SUSHI 3.0 release.

@cmoesel
Copy link
Member

cmoesel commented Jun 29, 2022

I've just run the regression. Everything looks right (good job!) -- but since it is a change in output, I do think we want to hold it for SUSHI 3.0. I will change the target of the PR to next. Hopefully there are not any conflicts! 🤞

@cmoesel
Copy link
Member

cmoesel commented Jun 29, 2022

Actually, I'm going to wait to change the target until after Julia's rebase of next gets in. Stand by.

@cmoesel cmoesel changed the base branch from master to next June 29, 2022 19:11
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 think the code still looks good, but the tests need a little tweaking.

test/export/InstanceExporter.test.ts Outdated Show resolved Hide resolved
test/export/InstanceExporter.test.ts Outdated Show resolved Hide resolved
test/export/InstanceExporter.test.ts Outdated Show resolved Hide resolved
test/export/InstanceExporter.test.ts Outdated Show resolved Hide resolved
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 made one last commit so that the id/title/description for the Patient test case talk about a patient instead of DemoQuestionnaire.

It all looks good now!

@cmoesel cmoesel merged commit bb343df into next Jul 20, 2022
@cmoesel cmoesel deleted the cimpl-882-consider-putting-name branch July 20, 2022 19:04
jafeltra pushed a commit that referenced this pull request Oct 7, 2022
* Init commit

* Tests added

* Undeleted instancemeta, moved logic to be under the if statement for #definition usage, added tests

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Removed name and status for patient

* Update test case so id/title/description are consistent

Co-authored-by: gthuran <Suma1-dumont>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
mint-thompson added a commit that referenced this pull request Nov 30, 2022
* Combine named slices and soft indexing

When assigning values on an Instance, it may be useful to use a
combination of soft indices and named slices. When this happens, first
create all the slices that will need to exist, both named and unnamed.
Then, proceed to apply values from the InstanceOf StructureDefinition
and any AssignmentRules on the Instance. Prior to this change, the
ordering of operations would frequently result in elements being merged
together in undesired ways. This was due to both the way soft indices
were resolved, as well as the assignment of implied properties when
exporting an Instance. Soft index resolution now accounts for named
slices when determining the indices of unnamed slices. Implied
properties are assigned after the slices are created in the correct
order, which means that they are no longer prone to forcing implied
properties on those slices to appear as the earliest entries in the
list. The test added to InstanceExporter demonstrates the difference in
behavior.

* Do not rename choice types on StructureDefinitions (#972)

* Do not rename choice types on StructureDefinitions

A specific choice type on a StructureDefinition should not have its id
or path renamed. Instead, put a slice name on the choice element.
Specific choice types on Instances are not changed.

* Remove extra id assignment

* Move shortcut id function to where it is used

The shortcut id is only ever needed by setImpliedPropertiesOnInstance,
so the function to get the shortcut id is moved to be in the same file.
The constants used by this function are still in the ElementDefinition
file, since I feel like that is where I would expect to find them if I
were looking for them.

* Move choice type slice name postfixes

* Always include type on diff for choice element slice

The IG publisher requires that the differential element representing a
slice of a choice element always contain a type. This is the case even
if its type is the same as the type of the choice element. Add a check
when calculating the differential so that the type is always included
for these elements.

Added TODO note to ElementDefinitionType.fromJSON regarding undefined
properties. There may be upcoming changes to requirements regarding
these types, which are currently included due to the implementation
difference between how ElementDefinitionType's constructor and fromJSON
methods handle undefined properties.

* Update antlr4 to 4.9.2 (#933)

* Update antlr4 to 4.9.2

The updated antlr4 library generates classes with a different export
style. Update the imports of those classes to use the default import
rather than a destructured import.

The directory structure of the antlr4 package adds two new directories.
Add package path mapping to the TypeScript compiler configuration and
the Jest configuration.

* Update antlr4 to 4.9.3

Update year in test fixtures, since they haven't been run since last
year.

* Handle antlr path resolution at runtime

Setting paths in the tsconfig is good for helping tsc find modules, but
doesn't help node out at runtime. Add the tsconfig-paths module so that
the modules will resolve. This works, but it adds a new library, so I'm
upset about it.

* Resolve antlr4 submodules relative to app

* Fix path registration in index.ts

When using SUSHI as a module, the entry point is index, not app. So,
both files need to have the antlr4 path registration applied.

* Replace package loading with FPL (#1026)

* Remove load functionality that is replace with fhir-package-load library. Update tests and fixtures.

* Silence fhir-package-load logger in tests

* Use updated exported FHIRDefinition class from fhir-package-load

* Pass in a log function to loadDependency

* Rename to fhir-package-loader

* Remove import of FPL logger for tests

* Remove cloneJsonValues and addDefinition imports from FPL and replace functions

* Add fishForMetadata and corresponding tests back

* Update enum to use string values

* Keep track of packages loaded through local variable. Remove property from FHIRDefs.

* Remove unused loadPackages array. Fix stray comma.

* Use mergeDependency function

* Add FPL package

* Combine named slices and soft indexing

When assigning values on an Instance, it may be useful to use a
combination of soft indices and named slices. When this happens, first
create all the slices that will need to exist, both named and unnamed.
Then, proceed to apply values from the InstanceOf StructureDefinition
and any AssignmentRules on the Instance. Prior to this change, the
ordering of operations would frequently result in elements being merged
together in undesired ways. This was due to both the way soft indices
were resolved, as well as the assignment of implied properties when
exporting an Instance. Soft index resolution now accounts for named
slices when determining the indices of unnamed slices. Implied
properties are assigned after the slices are created in the correct
order, which means that they are no longer prone to forcing implied
properties on those slices to appear as the earliest entries in the
list. The test added to InstanceExporter demonstrates the difference in
behavior.

* Don't slice single-type choices (unless already sliced) (#1116)

If a user refers to a specific choice type (e.g., valueQuantity), but the choice (e.g., value[x]) only allows one type anyway, then don't create a type slicing and slice, because they're not necessary.

On the other hand, if the choice has already been sliced, then go ahead and create the slice for consistency.

* Handle null array elements when assigning complex values

* Add slice name enforcement to configuration

* Instance Exporter uses configured slice name enforcement

When resolving soft indexing, a boolean parameter can be passed to
indicate whether "strict soft indexing" should be used. The value of
this parameter is taken from the SUSHI configuration. When this value is
false (the default), the behavior is consistent with previous versions
of SUSHI. When this value is true, soft indexing is resolved by
enforcing the practice of referring to slices by name.

* Reslices in soft indexing increment parent slices

When a path uses soft indexing to increment the value of an element with
reslices, the path part with the soft index will contain multiple
slices. The tracked index for each less-sliced version of the element
should be incremented in order to maintain the correct index when
less-sliced versions of the element are used.

* Don't warn on numeric index usage when enforcing slice name usage

When enforcing slice name usage, referring to a numeric index
necessarily refers to an element that is not one of the named slices.
Therefore, it is not referring to a named slice via numeric index, and
there is no need to emit a warning.

* Allow name/description in Instances (#1104)

* Init commit

* Tests added

* Undeleted instancemeta, moved logic to be under the if statement for #definition usage, added tests

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Removed name and status for patient

* Update test case so id/title/description are consistent

Co-authored-by: gthuran <Suma1-dumont>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>

* 3.0.0 Beta 1 Candidate: Update Node and Dev Tools (#1127)

* Upgrade Node and Dev Tools

Upgrade Node to 16 (and NPM 8).
Upgrade TypeScript to 4.7.
Upgrade ts-node, jest, eslint, prettier, etc.

* 3.0.0-beta.1

* Allow reslices to fulfill base slice cardinality

When a slice with a minimum cardinality greater than 0 is resliced,
reslices should count towards fulfilling that cardinality.

Add logic to ensure that implied properties on a slice are set, even
when that slice's cardinality is fulfilled by reslices.

Fix typo in soft index conversion functions.

* Order IG resources based on config (#1118)

* Order IG resources based on config

If all generated resources are present in the config resources section,
use the order from that section when adding resources to the IG.
Otherwise, if all generated resources are present in the config groups
section, use the order from that section when adding resources to the
IG. If neither config section contains all resources, add the resources
sorted by title or id.

* Fix Minifier Dependency and Location Type (#1113)

Changed dependency html-minifier to html-minifier-terser

* Add test for using title to sort resource

* Sort resources after all resources are added

Resources in the IG are sorted after adding generated, configured, and
predefined resources. A temporary sort key attribute is added to the
resources when adding them. This attribute is used for the default
sorting method.

* Refactor IG resource sorting

Test if a sort should be used and use it in the same function. This
avoids having to iterate and search over the set of resources additional
times.

* Sort resources by name

The sort key that was used to sort IG resources is removed. Resources
are now sorted by name. The name attribute should always exist, but if
it doesn't, use the reference key instead.

* Default resource order is case-insensitive

Co-authored-by: Alex Walley <[email protected]>

* messy commit to share with julia

things are not working quite yet. but i promise. we are very close. we
are so very close.

* Update some tests to hopefully work better

* Change element ordering in test

* Correctly create helpyblock for paths with slicenames

* Failing test case for siblings with multiple slices causing extra entries to be added

* Add tests from reslicing to ensure duplicate entries aren't added

* Slice minimums include numeric indices

When calculating the minimum number of elements for sliced elements,
include numeric indices for parts of the path before the last part. This
helps avoid creating unnecessary sibling elements.

A side effect of this is that implicit extension slices created on
instances will no longer automatically fill in their URL on elements in
the list prior to the ones actually referred to in rules. However, both
the previous and new behaviors are equally erroneous, and indicate that
the author should revise their FSH to create valid extension elements.

Some cleanup still needed.

* Removing status and version presets on exported artifacts (#1143)

* Removing version default, changing status default to draft per Grahamm

* prettier fix

* Default status to draft in config, pull status from config for all exports

* prettier fiex

* Pulling config from tank rather that package

* Traverse SD elements to find implied properties

A new function for setting implied properties is added. Rather than
iterating over the set of rule paths and using those paths to find
elements, this function iterates over the elements. The rule paths are
still used for certain checks.

This implementation is not yet complete, and is in need of significant
cleanup. The new function is only used when exporting instances, so the
previous function will remain for now.

* Remove old setImpliedPropertiesOnInstance and replace with new version

* Farewell to helpy. Refactor create and determine slices functions

* Remove old sort function and rootRequired property

* Clean up setImpliedPropertiesOnInstance

* Clean up findConnectedElements

* Rename configuration property to manualSliceOrdering and move into instanceOptions

* Minor updates based on PR review

- Add comment above confusing line of code for primitive properties
- Add comments to distinguish the strict and non-strict convertSoftIndices functions
- Only check for slice indices on extension elements

* Access the correct array element with pop

* Remove code that is no longer reached or needed

* Create a variable for manualSliceOrdering config property

* Eliminate old copy and merge approach for assigning values on Instances

* Update getSlices to properly handle getting reslices from an already sliced element

* Rename strict to manualSliceOrdering for clarity

* Separate out slice tree related functions and add comments explaining why we need slice trees

* Add comment explaining sliceIndices and ruleIndices when we createUsefulSlices

* Add README file explaining the approach taken in setImpliedPropertiesOnInstance

* Clean up test

* Added a few formatting updates to new README file

* Revert "Eliminate old copy and merge approach for assigning values on Instances"

This reverts commit f8f39d8.

Co-authored-by: Julia Afeltra <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Guhan B. Thuran <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Alex Walley <[email protected]>
Co-authored-by: Julia Afeltra <[email protected]>
Co-authored-by: Julian Carter <[email protected]>
cmoesel added a commit that referenced this pull request Feb 10, 2023
* Init commit

* Tests added

* Undeleted instancemeta, moved logic to be under the if statement for #definition usage, added tests

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Removed name and status for patient

* Update test case so id/title/description are consistent

Co-authored-by: gthuran <Suma1-dumont>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
cmoesel added a commit that referenced this pull request Feb 10, 2023
* Combine named slices and soft indexing

When assigning values on an Instance, it may be useful to use a
combination of soft indices and named slices. When this happens, first
create all the slices that will need to exist, both named and unnamed.
Then, proceed to apply values from the InstanceOf StructureDefinition
and any AssignmentRules on the Instance. Prior to this change, the
ordering of operations would frequently result in elements being merged
together in undesired ways. This was due to both the way soft indices
were resolved, as well as the assignment of implied properties when
exporting an Instance. Soft index resolution now accounts for named
slices when determining the indices of unnamed slices. Implied
properties are assigned after the slices are created in the correct
order, which means that they are no longer prone to forcing implied
properties on those slices to appear as the earliest entries in the
list. The test added to InstanceExporter demonstrates the difference in
behavior.

* Do not rename choice types on StructureDefinitions (#972)

* Do not rename choice types on StructureDefinitions

A specific choice type on a StructureDefinition should not have its id
or path renamed. Instead, put a slice name on the choice element.
Specific choice types on Instances are not changed.

* Remove extra id assignment

* Move shortcut id function to where it is used

The shortcut id is only ever needed by setImpliedPropertiesOnInstance,
so the function to get the shortcut id is moved to be in the same file.
The constants used by this function are still in the ElementDefinition
file, since I feel like that is where I would expect to find them if I
were looking for them.

* Move choice type slice name postfixes

* Always include type on diff for choice element slice

The IG publisher requires that the differential element representing a
slice of a choice element always contain a type. This is the case even
if its type is the same as the type of the choice element. Add a check
when calculating the differential so that the type is always included
for these elements.

Added TODO note to ElementDefinitionType.fromJSON regarding undefined
properties. There may be upcoming changes to requirements regarding
these types, which are currently included due to the implementation
difference between how ElementDefinitionType's constructor and fromJSON
methods handle undefined properties.

* Update antlr4 to 4.9.2 (#933)

* Update antlr4 to 4.9.2

The updated antlr4 library generates classes with a different export
style. Update the imports of those classes to use the default import
rather than a destructured import.

The directory structure of the antlr4 package adds two new directories.
Add package path mapping to the TypeScript compiler configuration and
the Jest configuration.

* Update antlr4 to 4.9.3

Update year in test fixtures, since they haven't been run since last
year.

* Handle antlr path resolution at runtime

Setting paths in the tsconfig is good for helping tsc find modules, but
doesn't help node out at runtime. Add the tsconfig-paths module so that
the modules will resolve. This works, but it adds a new library, so I'm
upset about it.

* Resolve antlr4 submodules relative to app

* Fix path registration in index.ts

When using SUSHI as a module, the entry point is index, not app. So,
both files need to have the antlr4 path registration applied.

* Replace package loading with FPL (#1026)

* Remove load functionality that is replace with fhir-package-load library. Update tests and fixtures.

* Silence fhir-package-load logger in tests

* Use updated exported FHIRDefinition class from fhir-package-load

* Pass in a log function to loadDependency

* Rename to fhir-package-loader

* Remove import of FPL logger for tests

* Remove cloneJsonValues and addDefinition imports from FPL and replace functions

* Add fishForMetadata and corresponding tests back

* Update enum to use string values

* Keep track of packages loaded through local variable. Remove property from FHIRDefs.

* Remove unused loadPackages array. Fix stray comma.

* Use mergeDependency function

* Add FPL package

* Combine named slices and soft indexing

When assigning values on an Instance, it may be useful to use a
combination of soft indices and named slices. When this happens, first
create all the slices that will need to exist, both named and unnamed.
Then, proceed to apply values from the InstanceOf StructureDefinition
and any AssignmentRules on the Instance. Prior to this change, the
ordering of operations would frequently result in elements being merged
together in undesired ways. This was due to both the way soft indices
were resolved, as well as the assignment of implied properties when
exporting an Instance. Soft index resolution now accounts for named
slices when determining the indices of unnamed slices. Implied
properties are assigned after the slices are created in the correct
order, which means that they are no longer prone to forcing implied
properties on those slices to appear as the earliest entries in the
list. The test added to InstanceExporter demonstrates the difference in
behavior.

* Don't slice single-type choices (unless already sliced) (#1116)

If a user refers to a specific choice type (e.g., valueQuantity), but the choice (e.g., value[x]) only allows one type anyway, then don't create a type slicing and slice, because they're not necessary.

On the other hand, if the choice has already been sliced, then go ahead and create the slice for consistency.

* Handle null array elements when assigning complex values

* Add slice name enforcement to configuration

* Instance Exporter uses configured slice name enforcement

When resolving soft indexing, a boolean parameter can be passed to
indicate whether "strict soft indexing" should be used. The value of
this parameter is taken from the SUSHI configuration. When this value is
false (the default), the behavior is consistent with previous versions
of SUSHI. When this value is true, soft indexing is resolved by
enforcing the practice of referring to slices by name.

* Reslices in soft indexing increment parent slices

When a path uses soft indexing to increment the value of an element with
reslices, the path part with the soft index will contain multiple
slices. The tracked index for each less-sliced version of the element
should be incremented in order to maintain the correct index when
less-sliced versions of the element are used.

* Don't warn on numeric index usage when enforcing slice name usage

When enforcing slice name usage, referring to a numeric index
necessarily refers to an element that is not one of the named slices.
Therefore, it is not referring to a named slice via numeric index, and
there is no need to emit a warning.

* Allow name/description in Instances (#1104)

* Init commit

* Tests added

* Undeleted instancemeta, moved logic to be under the if statement for #definition usage, added tests

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Removed name and status for patient

* Update test case so id/title/description are consistent

Co-authored-by: gthuran <Suma1-dumont>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>

* 3.0.0 Beta 1 Candidate: Update Node and Dev Tools (#1127)

* Upgrade Node and Dev Tools

Upgrade Node to 16 (and NPM 8).
Upgrade TypeScript to 4.7.
Upgrade ts-node, jest, eslint, prettier, etc.

* 3.0.0-beta.1

* Allow reslices to fulfill base slice cardinality

When a slice with a minimum cardinality greater than 0 is resliced,
reslices should count towards fulfilling that cardinality.

Add logic to ensure that implied properties on a slice are set, even
when that slice's cardinality is fulfilled by reslices.

Fix typo in soft index conversion functions.

* Order IG resources based on config (#1118)

* Order IG resources based on config

If all generated resources are present in the config resources section,
use the order from that section when adding resources to the IG.
Otherwise, if all generated resources are present in the config groups
section, use the order from that section when adding resources to the
IG. If neither config section contains all resources, add the resources
sorted by title or id.

* Fix Minifier Dependency and Location Type (#1113)

Changed dependency html-minifier to html-minifier-terser

* Add test for using title to sort resource

* Sort resources after all resources are added

Resources in the IG are sorted after adding generated, configured, and
predefined resources. A temporary sort key attribute is added to the
resources when adding them. This attribute is used for the default
sorting method.

* Refactor IG resource sorting

Test if a sort should be used and use it in the same function. This
avoids having to iterate and search over the set of resources additional
times.

* Sort resources by name

The sort key that was used to sort IG resources is removed. Resources
are now sorted by name. The name attribute should always exist, but if
it doesn't, use the reference key instead.

* Default resource order is case-insensitive

Co-authored-by: Alex Walley <[email protected]>

* messy commit to share with julia

things are not working quite yet. but i promise. we are very close. we
are so very close.

* Update some tests to hopefully work better

* Change element ordering in test

* Correctly create helpyblock for paths with slicenames

* Failing test case for siblings with multiple slices causing extra entries to be added

* Add tests from reslicing to ensure duplicate entries aren't added

* Slice minimums include numeric indices

When calculating the minimum number of elements for sliced elements,
include numeric indices for parts of the path before the last part. This
helps avoid creating unnecessary sibling elements.

A side effect of this is that implicit extension slices created on
instances will no longer automatically fill in their URL on elements in
the list prior to the ones actually referred to in rules. However, both
the previous and new behaviors are equally erroneous, and indicate that
the author should revise their FSH to create valid extension elements.

Some cleanup still needed.

* Removing status and version presets on exported artifacts (#1143)

* Removing version default, changing status default to draft per Grahamm

* prettier fix

* Default status to draft in config, pull status from config for all exports

* prettier fiex

* Pulling config from tank rather that package

* Traverse SD elements to find implied properties

A new function for setting implied properties is added. Rather than
iterating over the set of rule paths and using those paths to find
elements, this function iterates over the elements. The rule paths are
still used for certain checks.

This implementation is not yet complete, and is in need of significant
cleanup. The new function is only used when exporting instances, so the
previous function will remain for now.

* Remove old setImpliedPropertiesOnInstance and replace with new version

* Farewell to helpy. Refactor create and determine slices functions

* Remove old sort function and rootRequired property

* Clean up setImpliedPropertiesOnInstance

* Clean up findConnectedElements

* Rename configuration property to manualSliceOrdering and move into instanceOptions

* Minor updates based on PR review

- Add comment above confusing line of code for primitive properties
- Add comments to distinguish the strict and non-strict convertSoftIndices functions
- Only check for slice indices on extension elements

* Access the correct array element with pop

* Remove code that is no longer reached or needed

* Create a variable for manualSliceOrdering config property

* Eliminate old copy and merge approach for assigning values on Instances

* Update getSlices to properly handle getting reslices from an already sliced element

* Rename strict to manualSliceOrdering for clarity

* Separate out slice tree related functions and add comments explaining why we need slice trees

* Add comment explaining sliceIndices and ruleIndices when we createUsefulSlices

* Add README file explaining the approach taken in setImpliedPropertiesOnInstance

* Clean up test

* Added a few formatting updates to new README file

* Revert "Eliminate old copy and merge approach for assigning values on Instances"

This reverts commit f8f39d8.

Co-authored-by: Julia Afeltra <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Guhan B. Thuran <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Alex Walley <[email protected]>
Co-authored-by: Julia Afeltra <[email protected]>
Co-authored-by: Julian Carter <[email protected]>
cmoesel added a commit that referenced this pull request Feb 21, 2023
* Init commit

* Tests added

* Undeleted instancemeta, moved logic to be under the if statement for #definition usage, added tests

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Removed name and status for patient

* Update test case so id/title/description are consistent

Co-authored-by: gthuran <Suma1-dumont>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
cmoesel added a commit that referenced this pull request Feb 21, 2023
* Combine named slices and soft indexing

When assigning values on an Instance, it may be useful to use a
combination of soft indices and named slices. When this happens, first
create all the slices that will need to exist, both named and unnamed.
Then, proceed to apply values from the InstanceOf StructureDefinition
and any AssignmentRules on the Instance. Prior to this change, the
ordering of operations would frequently result in elements being merged
together in undesired ways. This was due to both the way soft indices
were resolved, as well as the assignment of implied properties when
exporting an Instance. Soft index resolution now accounts for named
slices when determining the indices of unnamed slices. Implied
properties are assigned after the slices are created in the correct
order, which means that they are no longer prone to forcing implied
properties on those slices to appear as the earliest entries in the
list. The test added to InstanceExporter demonstrates the difference in
behavior.

* Do not rename choice types on StructureDefinitions (#972)

* Do not rename choice types on StructureDefinitions

A specific choice type on a StructureDefinition should not have its id
or path renamed. Instead, put a slice name on the choice element.
Specific choice types on Instances are not changed.

* Remove extra id assignment

* Move shortcut id function to where it is used

The shortcut id is only ever needed by setImpliedPropertiesOnInstance,
so the function to get the shortcut id is moved to be in the same file.
The constants used by this function are still in the ElementDefinition
file, since I feel like that is where I would expect to find them if I
were looking for them.

* Move choice type slice name postfixes

* Always include type on diff for choice element slice

The IG publisher requires that the differential element representing a
slice of a choice element always contain a type. This is the case even
if its type is the same as the type of the choice element. Add a check
when calculating the differential so that the type is always included
for these elements.

Added TODO note to ElementDefinitionType.fromJSON regarding undefined
properties. There may be upcoming changes to requirements regarding
these types, which are currently included due to the implementation
difference between how ElementDefinitionType's constructor and fromJSON
methods handle undefined properties.

* Update antlr4 to 4.9.2 (#933)

* Update antlr4 to 4.9.2

The updated antlr4 library generates classes with a different export
style. Update the imports of those classes to use the default import
rather than a destructured import.

The directory structure of the antlr4 package adds two new directories.
Add package path mapping to the TypeScript compiler configuration and
the Jest configuration.

* Update antlr4 to 4.9.3

Update year in test fixtures, since they haven't been run since last
year.

* Handle antlr path resolution at runtime

Setting paths in the tsconfig is good for helping tsc find modules, but
doesn't help node out at runtime. Add the tsconfig-paths module so that
the modules will resolve. This works, but it adds a new library, so I'm
upset about it.

* Resolve antlr4 submodules relative to app

* Fix path registration in index.ts

When using SUSHI as a module, the entry point is index, not app. So,
both files need to have the antlr4 path registration applied.

* Replace package loading with FPL (#1026)

* Remove load functionality that is replace with fhir-package-load library. Update tests and fixtures.

* Silence fhir-package-load logger in tests

* Use updated exported FHIRDefinition class from fhir-package-load

* Pass in a log function to loadDependency

* Rename to fhir-package-loader

* Remove import of FPL logger for tests

* Remove cloneJsonValues and addDefinition imports from FPL and replace functions

* Add fishForMetadata and corresponding tests back

* Update enum to use string values

* Keep track of packages loaded through local variable. Remove property from FHIRDefs.

* Remove unused loadPackages array. Fix stray comma.

* Use mergeDependency function

* Add FPL package

* Combine named slices and soft indexing

When assigning values on an Instance, it may be useful to use a
combination of soft indices and named slices. When this happens, first
create all the slices that will need to exist, both named and unnamed.
Then, proceed to apply values from the InstanceOf StructureDefinition
and any AssignmentRules on the Instance. Prior to this change, the
ordering of operations would frequently result in elements being merged
together in undesired ways. This was due to both the way soft indices
were resolved, as well as the assignment of implied properties when
exporting an Instance. Soft index resolution now accounts for named
slices when determining the indices of unnamed slices. Implied
properties are assigned after the slices are created in the correct
order, which means that they are no longer prone to forcing implied
properties on those slices to appear as the earliest entries in the
list. The test added to InstanceExporter demonstrates the difference in
behavior.

* Don't slice single-type choices (unless already sliced) (#1116)

If a user refers to a specific choice type (e.g., valueQuantity), but the choice (e.g., value[x]) only allows one type anyway, then don't create a type slicing and slice, because they're not necessary.

On the other hand, if the choice has already been sliced, then go ahead and create the slice for consistency.

* Handle null array elements when assigning complex values

* Add slice name enforcement to configuration

* Instance Exporter uses configured slice name enforcement

When resolving soft indexing, a boolean parameter can be passed to
indicate whether "strict soft indexing" should be used. The value of
this parameter is taken from the SUSHI configuration. When this value is
false (the default), the behavior is consistent with previous versions
of SUSHI. When this value is true, soft indexing is resolved by
enforcing the practice of referring to slices by name.

* Reslices in soft indexing increment parent slices

When a path uses soft indexing to increment the value of an element with
reslices, the path part with the soft index will contain multiple
slices. The tracked index for each less-sliced version of the element
should be incremented in order to maintain the correct index when
less-sliced versions of the element are used.

* Don't warn on numeric index usage when enforcing slice name usage

When enforcing slice name usage, referring to a numeric index
necessarily refers to an element that is not one of the named slices.
Therefore, it is not referring to a named slice via numeric index, and
there is no need to emit a warning.

* Allow name/description in Instances (#1104)

* Init commit

* Tests added

* Undeleted instancemeta, moved logic to be under the if statement for #definition usage, added tests

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Removed name and status for patient

* Update test case so id/title/description are consistent

Co-authored-by: gthuran <Suma1-dumont>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>

* 3.0.0 Beta 1 Candidate: Update Node and Dev Tools (#1127)

* Upgrade Node and Dev Tools

Upgrade Node to 16 (and NPM 8).
Upgrade TypeScript to 4.7.
Upgrade ts-node, jest, eslint, prettier, etc.

* 3.0.0-beta.1

* Allow reslices to fulfill base slice cardinality

When a slice with a minimum cardinality greater than 0 is resliced,
reslices should count towards fulfilling that cardinality.

Add logic to ensure that implied properties on a slice are set, even
when that slice's cardinality is fulfilled by reslices.

Fix typo in soft index conversion functions.

* Order IG resources based on config (#1118)

* Order IG resources based on config

If all generated resources are present in the config resources section,
use the order from that section when adding resources to the IG.
Otherwise, if all generated resources are present in the config groups
section, use the order from that section when adding resources to the
IG. If neither config section contains all resources, add the resources
sorted by title or id.

* Fix Minifier Dependency and Location Type (#1113)

Changed dependency html-minifier to html-minifier-terser

* Add test for using title to sort resource

* Sort resources after all resources are added

Resources in the IG are sorted after adding generated, configured, and
predefined resources. A temporary sort key attribute is added to the
resources when adding them. This attribute is used for the default
sorting method.

* Refactor IG resource sorting

Test if a sort should be used and use it in the same function. This
avoids having to iterate and search over the set of resources additional
times.

* Sort resources by name

The sort key that was used to sort IG resources is removed. Resources
are now sorted by name. The name attribute should always exist, but if
it doesn't, use the reference key instead.

* Default resource order is case-insensitive

Co-authored-by: Alex Walley <[email protected]>

* messy commit to share with julia

things are not working quite yet. but i promise. we are very close. we
are so very close.

* Update some tests to hopefully work better

* Change element ordering in test

* Correctly create helpyblock for paths with slicenames

* Failing test case for siblings with multiple slices causing extra entries to be added

* Add tests from reslicing to ensure duplicate entries aren't added

* Slice minimums include numeric indices

When calculating the minimum number of elements for sliced elements,
include numeric indices for parts of the path before the last part. This
helps avoid creating unnecessary sibling elements.

A side effect of this is that implicit extension slices created on
instances will no longer automatically fill in their URL on elements in
the list prior to the ones actually referred to in rules. However, both
the previous and new behaviors are equally erroneous, and indicate that
the author should revise their FSH to create valid extension elements.

Some cleanup still needed.

* Removing status and version presets on exported artifacts (#1143)

* Removing version default, changing status default to draft per Grahamm

* prettier fix

* Default status to draft in config, pull status from config for all exports

* prettier fiex

* Pulling config from tank rather that package

* Traverse SD elements to find implied properties

A new function for setting implied properties is added. Rather than
iterating over the set of rule paths and using those paths to find
elements, this function iterates over the elements. The rule paths are
still used for certain checks.

This implementation is not yet complete, and is in need of significant
cleanup. The new function is only used when exporting instances, so the
previous function will remain for now.

* Remove old setImpliedPropertiesOnInstance and replace with new version

* Farewell to helpy. Refactor create and determine slices functions

* Remove old sort function and rootRequired property

* Clean up setImpliedPropertiesOnInstance

* Clean up findConnectedElements

* Rename configuration property to manualSliceOrdering and move into instanceOptions

* Minor updates based on PR review

- Add comment above confusing line of code for primitive properties
- Add comments to distinguish the strict and non-strict convertSoftIndices functions
- Only check for slice indices on extension elements

* Access the correct array element with pop

* Remove code that is no longer reached or needed

* Create a variable for manualSliceOrdering config property

* Eliminate old copy and merge approach for assigning values on Instances

* Update getSlices to properly handle getting reslices from an already sliced element

* Rename strict to manualSliceOrdering for clarity

* Separate out slice tree related functions and add comments explaining why we need slice trees

* Add comment explaining sliceIndices and ruleIndices when we createUsefulSlices

* Add README file explaining the approach taken in setImpliedPropertiesOnInstance

* Clean up test

* Added a few formatting updates to new README file

* Revert "Eliminate old copy and merge approach for assigning values on Instances"

This reverts commit f8f39d8.

Co-authored-by: Julia Afeltra <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Guhan B. Thuran <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Alex Walley <[email protected]>
Co-authored-by: Julia Afeltra <[email protected]>
Co-authored-by: Julian Carter <[email protected]>
cmoesel added a commit that referenced this pull request Feb 24, 2023
* Init commit

* Tests added

* Undeleted instancemeta, moved logic to be under the if statement for #definition usage, added tests

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Removed name and status for patient

* Update test case so id/title/description are consistent

Co-authored-by: gthuran <Suma1-dumont>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
cmoesel added a commit that referenced this pull request Feb 24, 2023
* Combine named slices and soft indexing

When assigning values on an Instance, it may be useful to use a
combination of soft indices and named slices. When this happens, first
create all the slices that will need to exist, both named and unnamed.
Then, proceed to apply values from the InstanceOf StructureDefinition
and any AssignmentRules on the Instance. Prior to this change, the
ordering of operations would frequently result in elements being merged
together in undesired ways. This was due to both the way soft indices
were resolved, as well as the assignment of implied properties when
exporting an Instance. Soft index resolution now accounts for named
slices when determining the indices of unnamed slices. Implied
properties are assigned after the slices are created in the correct
order, which means that they are no longer prone to forcing implied
properties on those slices to appear as the earliest entries in the
list. The test added to InstanceExporter demonstrates the difference in
behavior.

* Do not rename choice types on StructureDefinitions (#972)

* Do not rename choice types on StructureDefinitions

A specific choice type on a StructureDefinition should not have its id
or path renamed. Instead, put a slice name on the choice element.
Specific choice types on Instances are not changed.

* Remove extra id assignment

* Move shortcut id function to where it is used

The shortcut id is only ever needed by setImpliedPropertiesOnInstance,
so the function to get the shortcut id is moved to be in the same file.
The constants used by this function are still in the ElementDefinition
file, since I feel like that is where I would expect to find them if I
were looking for them.

* Move choice type slice name postfixes

* Always include type on diff for choice element slice

The IG publisher requires that the differential element representing a
slice of a choice element always contain a type. This is the case even
if its type is the same as the type of the choice element. Add a check
when calculating the differential so that the type is always included
for these elements.

Added TODO note to ElementDefinitionType.fromJSON regarding undefined
properties. There may be upcoming changes to requirements regarding
these types, which are currently included due to the implementation
difference between how ElementDefinitionType's constructor and fromJSON
methods handle undefined properties.

* Update antlr4 to 4.9.2 (#933)

* Update antlr4 to 4.9.2

The updated antlr4 library generates classes with a different export
style. Update the imports of those classes to use the default import
rather than a destructured import.

The directory structure of the antlr4 package adds two new directories.
Add package path mapping to the TypeScript compiler configuration and
the Jest configuration.

* Update antlr4 to 4.9.3

Update year in test fixtures, since they haven't been run since last
year.

* Handle antlr path resolution at runtime

Setting paths in the tsconfig is good for helping tsc find modules, but
doesn't help node out at runtime. Add the tsconfig-paths module so that
the modules will resolve. This works, but it adds a new library, so I'm
upset about it.

* Resolve antlr4 submodules relative to app

* Fix path registration in index.ts

When using SUSHI as a module, the entry point is index, not app. So,
both files need to have the antlr4 path registration applied.

* Replace package loading with FPL (#1026)

* Remove load functionality that is replace with fhir-package-load library. Update tests and fixtures.

* Silence fhir-package-load logger in tests

* Use updated exported FHIRDefinition class from fhir-package-load

* Pass in a log function to loadDependency

* Rename to fhir-package-loader

* Remove import of FPL logger for tests

* Remove cloneJsonValues and addDefinition imports from FPL and replace functions

* Add fishForMetadata and corresponding tests back

* Update enum to use string values

* Keep track of packages loaded through local variable. Remove property from FHIRDefs.

* Remove unused loadPackages array. Fix stray comma.

* Use mergeDependency function

* Add FPL package

* Combine named slices and soft indexing

When assigning values on an Instance, it may be useful to use a
combination of soft indices and named slices. When this happens, first
create all the slices that will need to exist, both named and unnamed.
Then, proceed to apply values from the InstanceOf StructureDefinition
and any AssignmentRules on the Instance. Prior to this change, the
ordering of operations would frequently result in elements being merged
together in undesired ways. This was due to both the way soft indices
were resolved, as well as the assignment of implied properties when
exporting an Instance. Soft index resolution now accounts for named
slices when determining the indices of unnamed slices. Implied
properties are assigned after the slices are created in the correct
order, which means that they are no longer prone to forcing implied
properties on those slices to appear as the earliest entries in the
list. The test added to InstanceExporter demonstrates the difference in
behavior.

* Don't slice single-type choices (unless already sliced) (#1116)

If a user refers to a specific choice type (e.g., valueQuantity), but the choice (e.g., value[x]) only allows one type anyway, then don't create a type slicing and slice, because they're not necessary.

On the other hand, if the choice has already been sliced, then go ahead and create the slice for consistency.

* Handle null array elements when assigning complex values

* Add slice name enforcement to configuration

* Instance Exporter uses configured slice name enforcement

When resolving soft indexing, a boolean parameter can be passed to
indicate whether "strict soft indexing" should be used. The value of
this parameter is taken from the SUSHI configuration. When this value is
false (the default), the behavior is consistent with previous versions
of SUSHI. When this value is true, soft indexing is resolved by
enforcing the practice of referring to slices by name.

* Reslices in soft indexing increment parent slices

When a path uses soft indexing to increment the value of an element with
reslices, the path part with the soft index will contain multiple
slices. The tracked index for each less-sliced version of the element
should be incremented in order to maintain the correct index when
less-sliced versions of the element are used.

* Don't warn on numeric index usage when enforcing slice name usage

When enforcing slice name usage, referring to a numeric index
necessarily refers to an element that is not one of the named slices.
Therefore, it is not referring to a named slice via numeric index, and
there is no need to emit a warning.

* Allow name/description in Instances (#1104)

* Init commit

* Tests added

* Undeleted instancemeta, moved logic to be under the if statement for #definition usage, added tests

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Update test/export/InstanceExporter.test.ts

Co-authored-by: Chris Moesel <[email protected]>

* Removed name and status for patient

* Update test case so id/title/description are consistent

Co-authored-by: gthuran <Suma1-dumont>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>

* 3.0.0 Beta 1 Candidate: Update Node and Dev Tools (#1127)

* Upgrade Node and Dev Tools

Upgrade Node to 16 (and NPM 8).
Upgrade TypeScript to 4.7.
Upgrade ts-node, jest, eslint, prettier, etc.

* 3.0.0-beta.1

* Allow reslices to fulfill base slice cardinality

When a slice with a minimum cardinality greater than 0 is resliced,
reslices should count towards fulfilling that cardinality.

Add logic to ensure that implied properties on a slice are set, even
when that slice's cardinality is fulfilled by reslices.

Fix typo in soft index conversion functions.

* Order IG resources based on config (#1118)

* Order IG resources based on config

If all generated resources are present in the config resources section,
use the order from that section when adding resources to the IG.
Otherwise, if all generated resources are present in the config groups
section, use the order from that section when adding resources to the
IG. If neither config section contains all resources, add the resources
sorted by title or id.

* Fix Minifier Dependency and Location Type (#1113)

Changed dependency html-minifier to html-minifier-terser

* Add test for using title to sort resource

* Sort resources after all resources are added

Resources in the IG are sorted after adding generated, configured, and
predefined resources. A temporary sort key attribute is added to the
resources when adding them. This attribute is used for the default
sorting method.

* Refactor IG resource sorting

Test if a sort should be used and use it in the same function. This
avoids having to iterate and search over the set of resources additional
times.

* Sort resources by name

The sort key that was used to sort IG resources is removed. Resources
are now sorted by name. The name attribute should always exist, but if
it doesn't, use the reference key instead.

* Default resource order is case-insensitive

Co-authored-by: Alex Walley <[email protected]>

* messy commit to share with julia

things are not working quite yet. but i promise. we are very close. we
are so very close.

* Update some tests to hopefully work better

* Change element ordering in test

* Correctly create helpyblock for paths with slicenames

* Failing test case for siblings with multiple slices causing extra entries to be added

* Add tests from reslicing to ensure duplicate entries aren't added

* Slice minimums include numeric indices

When calculating the minimum number of elements for sliced elements,
include numeric indices for parts of the path before the last part. This
helps avoid creating unnecessary sibling elements.

A side effect of this is that implicit extension slices created on
instances will no longer automatically fill in their URL on elements in
the list prior to the ones actually referred to in rules. However, both
the previous and new behaviors are equally erroneous, and indicate that
the author should revise their FSH to create valid extension elements.

Some cleanup still needed.

* Removing status and version presets on exported artifacts (#1143)

* Removing version default, changing status default to draft per Grahamm

* prettier fix

* Default status to draft in config, pull status from config for all exports

* prettier fiex

* Pulling config from tank rather that package

* Traverse SD elements to find implied properties

A new function for setting implied properties is added. Rather than
iterating over the set of rule paths and using those paths to find
elements, this function iterates over the elements. The rule paths are
still used for certain checks.

This implementation is not yet complete, and is in need of significant
cleanup. The new function is only used when exporting instances, so the
previous function will remain for now.

* Remove old setImpliedPropertiesOnInstance and replace with new version

* Farewell to helpy. Refactor create and determine slices functions

* Remove old sort function and rootRequired property

* Clean up setImpliedPropertiesOnInstance

* Clean up findConnectedElements

* Rename configuration property to manualSliceOrdering and move into instanceOptions

* Minor updates based on PR review

- Add comment above confusing line of code for primitive properties
- Add comments to distinguish the strict and non-strict convertSoftIndices functions
- Only check for slice indices on extension elements

* Access the correct array element with pop

* Remove code that is no longer reached or needed

* Create a variable for manualSliceOrdering config property

* Eliminate old copy and merge approach for assigning values on Instances

* Update getSlices to properly handle getting reslices from an already sliced element

* Rename strict to manualSliceOrdering for clarity

* Separate out slice tree related functions and add comments explaining why we need slice trees

* Add comment explaining sliceIndices and ruleIndices when we createUsefulSlices

* Add README file explaining the approach taken in setImpliedPropertiesOnInstance

* Clean up test

* Added a few formatting updates to new README file

* Revert "Eliminate old copy and merge approach for assigning values on Instances"

This reverts commit f8f39d8.

Co-authored-by: Julia Afeltra <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Guhan B. Thuran <[email protected]>
Co-authored-by: Chris Moesel <[email protected]>
Co-authored-by: Alex Walley <[email protected]>
Co-authored-by: Julia Afeltra <[email protected]>
Co-authored-by: Julian Carter <[email protected]>
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.

Consider populating title/description/name(?) in instances when possible
3 participants