Stream: committers
Topic: isModifierReason
Grahame Grieve (Dec 14 2017 at 06:16):
GF#13065 proposes the creation of isModifierReason - and that it be mandatory when isModifier is true... yay. @Rick Geimer and @Richard Ettema you have your names on this one - did the committee discuss how that field would get to be populated?
Grahame Grieve (Dec 14 2017 at 06:19):
and if co-occurs with isModifier... why not get rid of isModifier? @Lloyd McKenzie
Lloyd McKenzie (Dec 14 2017 at 15:53):
Agree getting rid of isModifierReason would make sense. I guess we'd change the spreadsheet column from Y/N to a string
Lloyd McKenzie (Dec 14 2017 at 15:54):
Probably a bit late to do before this ballot...
Lloyd McKenzie (Dec 14 2017 at 15:54):
Note Grahame that you were the submitter :)
Richard Ettema (Dec 14 2017 at 16:01):
I'm looking at this now in my local build environment. Added isModifierReason with corresponding invariant and just finished a build. No errors.
Richard Ettema (Dec 14 2017 at 16:04):
If isModifier is instead changed to string, that would mean the value would already be populated with 'true'.
Eric Haas (Dec 14 2017 at 16:05):
what would the string be for say a status?....
Richard Ettema (Dec 14 2017 at 16:06):
From a tooling perspective wouldn't it make more sense to leave the isModifier boolean? Note, from my last build it's pretty obvious which StructureDefinitions need to have the isModifierReason populated.
Eric Haas (Dec 14 2017 at 16:08):
Or a set of codeableConcepts so every editor is not making up stuff like "its a status!"
Richard Ettema (Dec 14 2017 at 16:08):
@Eric Haas I guess it depends on the data type of the status; e.g. code, string, Coding, etc.
Eric Haas (Dec 14 2017 at 16:08):
or "this is a game changer."
Richard Ettema (Dec 14 2017 at 16:10):
If we follow the gForge ticket (which I am by the way), it would not be a game changer. The tooling would continue to work and we would just need to populate the new isModifierReason description element.
Eric Haas (Dec 14 2017 at 16:31):
sorry that was meant to be an example for the text in isModifierReason :-)
Richard Ettema (Dec 14 2017 at 16:40):
;-)
Lloyd McKenzie (Dec 14 2017 at 16:42):
status would say "enteredInError changes the interpretation of the entire resource"
Eric Haas (Dec 14 2017 at 16:44):
My point is that is what you would write. I would write "the status could be a game changer" so codeable with extensible binding makes more sense to me . And keeping it boolean makes the most sense to me.
Lloyd McKenzie (Dec 14 2017 at 16:52):
The intention is for the string to explain why it's a game-changer. Most of the statuses aren't actually modifiers. It's generally only entered-in-error. The purpose of the string is to identify what actually modifies the interpretation of the resource/containing element and how. That provides more guidance to implementers about when/whether they can safely ignore the modifier.
Lloyd McKenzie (Dec 14 2017 at 16:53):
The agreement to capture the reason has already been approved - what we're discussing now is how to implement that.
Eric Haas (Dec 14 2017 at 16:53):
But the proposal was to remove th@**Lloyd McKenzie
". why not get rid of isModifier? @Lloyd McKenzie"
Eric Haas (Dec 14 2017 at 16:55):
I think just having the reason is not a good idea
Lloyd McKenzie (Dec 14 2017 at 16:57):
If a reason is present, it's a modifier. If there's no reason, it's not a modifier. The boolean is sort of redundant if the reason is mandatory for modifiers.
Eric Haas (Dec 14 2017 at 16:59):
OK back to point 1) The reasons for the same thing (e.g., status) will be all over the place for core committers if left to our own devices. Hence a needed for a valueset of canned reasons.
Lloyd McKenzie (Dec 14 2017 at 17:29):
I don't know that we need a value set. However, a couple of canned phrases for the modifiers that are common should work - which can probably come from the resource patterns
Richard Ettema (Dec 14 2017 at 17:39):
Ok. I guess what I'm hearing from this thread is
- Lloyd is recommending we do not add a new element (isModifierReason) and change the current isModifier element to string. Not sure what impact this will have on the tooling.
- Eric (and I) like the idea of keeping isModifier as a boolean. The new element isModifierReason would become a CodeableConcept which allows for a text description w/o any specific codes; however, we would need to define an appropriate set of recommended codes.
- From my perspective both of these violate the approved direction as given in the gForge ticket GF#13065. At this point I would recommend that we either agree to the appoved direction, or throw this ticket back to a status of 'Triaged' with appropriate comments taken from this chat thread.
Thoughts?
Eric Haas (Dec 14 2017 at 17:44):
I think it needs more time too
Lloyd McKenzie (Dec 14 2017 at 17:51):
I'm not understanding the rationale for retaining a boolean. And I'm not really understanding the rationale for a code system whose sole purpose is to get consistency across resources when we already have other mechanisms to do that. (And technically it's Grahame whose proposing getting rid of the boolean - I just concurred that it was reasonable :))
Eric Haas (Dec 14 2017 at 17:53):
I think the point is that what is in the tracker is not what is being proposed so need some clarity
Richard Ettema (Dec 14 2017 at 17:58):
Agreed. Based on Grahame's original comment and the discussion we have just had, it would seem that the current opinion on how to implement this change is not in agreement with the approved direction. So, I would recommend we add this discussion thread to the gForge ticket and bring it back up for review.
Rick Geimer (Dec 14 2017 at 17:59):
If we are not going to implement it per the tracker, I think we should reopen that tracker and defer, because I would not like to see a potentially breaking change this close to the ballot.
Lloyd McKenzie (Dec 14 2017 at 18:33):
This is a change we need to make for R4. It's not a change we can make before this Sunday, but it's absolutely a change we need to make before April, so deferring isn't really an option.
Eric Haas (Dec 14 2017 at 18:57):
I don't think is an issue for April
Grahame Grieve (Dec 14 2017 at 20:24):
depends what you think deferring is.
Grahame Grieve (Dec 14 2017 at 20:25):
to be clear, we do not need a codeable concept to impose consistency. we have lots of ways to do that
Grahame Grieve (Dec 14 2017 at 20:25):
but there's no impact to process between adding isModifierReason, or changing isModifier to a string. for the build tools, it makes no difference to workload
Eric Haas (Dec 14 2017 at 20:29):
deferred = after the for comment ballot - I think is unsettled for immediate application
Richard Ettema (Dec 14 2017 at 20:39):
FYI - I've successfully implemented this change (locally) per the gForge ticket. I reviewed all of the StructureDefinition instances generated by the build - either explicitly defined as an XML StructureDefinition instance or as an Excel spreadsheet - and found close to 1,200 instances of ElementDefinitions with isModifier=true. In order for the new invariant "if the element is a modifier, there must be a reason description" to be satisfied, the new isModifierReason element will need to be populated for these ElementDefinitions.
Grahame Grieve (Dec 14 2017 at 20:41):
the master source is a little less than 1200: http://build.fhir.org/conformance-rules.html#isModifier
Grahame Grieve (Dec 14 2017 at 20:41):
but we have to go through and actually describe the reason for all those elements
Richard Ettema (Dec 14 2017 at 20:45):
I've noticed that the element comments in the spreadsheets are typically filled in for these and appear to define the 'reason' for isModifier=true. That might be good place to start.
Grahame Grieve (Dec 14 2017 at 20:45):
sure, it's just work. There's no magic here
Richard Ettema (Dec 14 2017 at 20:48):
So, where do we/I go from here? Do you want this change committed before the ballot deadline or should this wait until after?
Grahame Grieve (Dec 14 2017 at 20:50):
what's 'this change'? speciifcally - with regard to my first questions?
Richard Ettema (Dec 14 2017 at 20:52):
The change per the gForge ticket - add 'isModifierReason 0..1 string' with new invariant to require isModifierReason if isModifier = true
Grahame Grieve (Dec 14 2017 at 21:06):
so how did you populate isModifierReason?
Richard Ettema (Dec 14 2017 at 21:11):
I didn't. The build was successful without doing that. Apparently the build process does not validate the generated StructureDefinitions.
Grahame Grieve (Dec 14 2017 at 21:12):
I believe it does. you might have missed something - what's your expression for the co-occurence constraint?
Richard Ettema (Dec 14 2017 at 21:12):
(isModifier.toString() = 'false') or isModifierReason.exists()
Grahame Grieve (Dec 14 2017 at 21:20):
that looks right. though isModifier.not() would be cleaner
Richard Ettema (Dec 14 2017 at 21:24):
I think I know why. I have to copy to new generated Java classes from build/temp/java to the build/implementations/java folder, right?
Grahame Grieve (Dec 14 2017 at 21:27):
no
Grahame Grieve (Dec 14 2017 at 21:27):
don't do that
Richard Ettema (Dec 14 2017 at 21:29):
Ok. I found the code in Publisher.java where its validating the generated profiles, so I don't quite understand how my build is successful.
Richard Ettema (Dec 14 2017 at 21:35):
Nevermind, I just examined the ProfileValidator.validate() method and it's only checking extensions, snapshot exists and the FHIRPath expressions are valid. It doesn't look like the ValidationEngine is being called for the generated profiles.
Grahame Grieve (Dec 14 2017 at 22:53):
now I realise that this is also a change that requires community consultation
Lloyd McKenzie (Dec 14 2017 at 23:23):
A "status" of deferred means deferred until after the next publication. So we don't want to change the status.
Lloyd McKenzie (Dec 14 2017 at 23:56):
For the community consultation ones on candidate normative artifacts, do we want to include notes to balloters?
Grahame Grieve (Dec 15 2017 at 00:16):
some of the changes represent several days of work for me. I'm not making them before consultation
Grahame Grieve (Dec 15 2017 at 00:18):
but given that they won't make the draft for comment anyway, I'm going to wait to till the ballot opens and then do community consultation while it's open. there's 7 tasks at this point
Lloyd McKenzie (Dec 15 2017 at 00:20):
I wasn't suggesting applying them - just including a note in the ballot intro that draws attention to the proposed changes.
Grahame Grieve (Dec 15 2017 at 00:24):
well, we can do that
Grahame Grieve (Dec 18 2017 at 11:41):
so I never got back to this - sorry - not sure what to do with it
Richard Ettema (Dec 18 2017 at 14:25):
FYI - I did get this working in my local build where I updated the ProfileGenerator logic to write the ElementDefinition.comment value to the new isModifierReason element; and where comment is empty, write the following string --> ElementDefinition.path + " changes the interpretation of the entire resource".
Thoughts?
Grahame Grieve (Dec 18 2017 at 18:16):
I don't see the point of creating ti, and then generating an abstract comment that brings no incision
Richard Ettema (Dec 18 2017 at 18:31):
Ok. Lloyd has updated the tracker's Group to "Community Consultation". So, I guess that means it's back up for review/discussion by FHIR-I
Lloyd McKenzie (Dec 18 2017 at 18:43):
Wasn't me...
Richard Ettema (Dec 18 2017 at 18:46):
Sorry. It was Grahame.
Last updated: Apr 12 2022 at 19:14 UTC
