Support "position" slicing discriminator #1538
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FHIR R5 added "position" as an allowed slice discriminator, where slices are differentiated by their order within a list element. Because a consistent order is necessary for this slicing to be meaningful, emit an error if "ordered" is not set to true for the slicing. Previous versions of FHIR may not use this discriminator. Minor changes to npm-shrinkwrap.json due to issues detected by `npm audit`.
jafeltra
approved these changes
Nov 22, 2024
cmoesel
approved these changes
Nov 26, 2024
Comment on lines
+401
to
+408
| if (d.type === 'position' && slicing.ordered !== true) { | ||
| validationErrors.push( | ||
| new ValidationError( | ||
| 'Slicing ordering must be true when a position discriminator is used.', | ||
| 'slicing.ordered' | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Here's something to think about: If the user uses a position discriminator type, should we just set ordered to true for them? I.e., we know what it should be, so... just do it? Putting the "short" in "shorthand"?
I'm torn on it. I feel like if they don't set the ordered property themself using a caret rule, then it makes sense to set it for them. But if they had set it themself to the wrong value (false), then I'm not sure if silently setting it to true is the right thing to do...
Comment on lines
+3094
to
+3101
| const ALLOWED_DISCRIMINATOR_TYPES_R5 = [ | ||
| 'value', | ||
| 'exists', | ||
| 'pattern', | ||
| 'type', | ||
| 'profile', | ||
| 'position' | ||
| ]; |
There was a problem hiding this comment.
I don't have a strong opinion on this, but... it could be:
Suggested change
| const ALLOWED_DISCRIMINATOR_TYPES_R5 = [ | |
| 'value', | |
| 'exists', | |
| 'pattern', | |
| 'type', | |
| 'profile', | |
| 'position' | |
| ]; | |
| const ALLOWED_DISCRIMINATOR_TYPES_R5 = [...ALLOWED_DISCRIMINATOR_TYPES, 'position']; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
FHIR R5 added "position" as an allowed slice discriminator, where slices are differentiated by their order within a list element. Because a consistent order is necessary for this slicing to be meaningful, emit an error if "ordered" is not set to true for the slicing. Previous versions of FHIR may not use this discriminator.
Minor changes to npm-shrinkwrap.json due to issues detected by
npm audit.Testing Instructions:
Confirm that new tests adequately cover this new feature. Try building projects with R4 and R5 resources using SUSHI, observing that the "position" discriminator can't be used with R4, but can be used with R5. There is also a new error message, so please let me know if you have suggestions for its wording.
Related Issue:
Partially addresses #1391, but does not validate Instances to confirm that slices are provided in the required order when position-based slicing is used.