-
Notifications
You must be signed in to change notification settings - Fork 43
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
Account for pattern on parent #219
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stuff is tricky, isn't it? I have a question about the behavior of arrays. See individual comment below.
medicationForm.fixValue(new FshCode('foo', 'http://thankYouForSettingMe.com')); | ||
const medicationFormCodingSystem = medication.findElementByPath('form.coding.system', fisher); | ||
expect(() => { | ||
medicationFormCodingSystem.fixValue('http://ohManIWillNeverGetSet.sad'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you just checking to see if I actually read these PRs? ;-)
// Multiple matching array elements | ||
statusReason.patternCodeableConcept = { coding: [{ system: 'foo' }, { system: 'foo' }] }; | ||
patternValue = statusReasonCodingSystem.fixedByAnyParent(); | ||
expect(patternValue).toBe('foo'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to figure out how the implications of this test (and the behavior behind it) play out in the real world. It kind of seems like this:
When I am trying to fix an array element:
- If a parent pattern sets a single value in the array, I will get an error unless my value matches the single value.
- If a parent pattern sets multiple, but equal, values in the array, I will get an error unless my value matches the value repeated in the pattern.
- If a parent pattern sets mutliple different values in the array, I will not get an error? SUSHI will let me set a new value?
If that's how it works, it seems a bit inconsistent to me -- but maybe you can explain the reasoning for that approach? Or maybe I'm understanding it wrong...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you make a good point. I guess my original thinking was that when multiple different values are set in an array, the fixedByAnyParent
function cannot determine any single fixed value, so we return undefined. But you're right, that is an issue because then there is no way to differentiate between undefined because no value is set, and undefined because conflicting values are set, and those cases need to be handled very differently.
So then the question is, what to return when there are multiple conflicting values? I'm actually not totally sure, I need to think about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you're flattening arrays into a single value before you return them? If the path is fixed to an array, why not return the array? And then let the code that calls fixedByAnyParent
decide what to do based on context? Or does that mess up the approach to instances?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the value itself is fixed to an array by a pattern, that does not get flattened. So as an example, let's say code
is fixed as:
patternCodeableConcept: {
coding: [ { system: 'foo' }, { system: 'bar' }]
}
Then code.coding
is fixed to [ { system: 'foo' }, { system: 'bar' }]
, and that is what gets returned by the fixedByAnyParent
function. But when we run fixedByAnyParent
on code.coding.system
I didn't think there was a clear choice for what we should return. Since system
is set to two different values in the different array elements.
My thinking was, if all the system
s in the array match, and they also match the new value you are trying to set, that is fine. If the system
s in the array do not match, then you can't really set system
to anything else, because no matter what you fixed it to, that would conflict with one of the values dictated by the pattern. So it was in this situation that I returned undefined
, but I now see that doesn't entirely work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes. Right. Got it.
I still think maybe we should think about returning an array. I suppose we have two possibilities:
-
Always return an array of the collected values for the path -- so in the example above,
code.coding.system
would result in['foo', 'bar']
. In an example where the systems are the same, it would be['foo', 'foo', 'foo']
. In an example where only one is set, it would still be['foo']
(since there is an array in its path). Then the calling code handles it -- probably by checking that every element in the array matches the new fixed value. That would handle all those cases correctly. -
Still return a single value when there is only one or all the values are equal. But in cases where they are different, return an array of the different values. Then when the code checks to see if the new fixed value equals the old one, it will fail because an array never equals a single item.
Anyway, those are a couple of ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we'd flatten to a single one-dimensional array, but I see your point. I guess the question is if there is a case where we need to know the difference? I guess maybe when generating instances?
As for [['foo', 'bar']]
, I'm assuming that means that the tail of the path is an array, but so is one of it's parent elements? In other words, however many dimensions the array is, that is how many array elements are in its path. But if there is only one element in the path that is an array, then it is a one-dimension array (e.g., [ 'foo', 'bar' ]
) no matter where the array is introduced in the path, right? If so, I'm down with that if you don't think it will be too hard to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it too hacky for you if I just return []
instead of undefined
when multiple array elements exist that conflict? I think this will work fine because []
is never going to equal the value the user is trying to set, but it isn't undefined
, so the user won't be able to reset the value. The approaches we have been discussing are less hacky, but complicated enough to implement that I'm not sure it's worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Scrub that from your mind, won't work well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like ... option 2
Me ^
Yeah so I went with option 2 in the latest commit, I think it'll work well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Fixes #84.
Previously when fixing values, we did not account for values being fixed via their parents by a pattern. This PR adds support for checking if a value is fixed by pattern from its parent, and we now throw an error if we try to re-set a value that is fixed by pattern.
If you'd like to test it out, fixing
CodeableConcept
elements works with pattern, so they provide a good test case. For example, the following will now generate an error, which was not the case with master: