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

2936 Make comparisons consistent #3017

Draft
wants to merge 6 commits into
base: develop-6.0
Choose a base branch
from

Conversation

ewoutkramer
Copy link
Member

@ewoutkramer ewoutkramer commented Jan 17, 2025

This PR removes comparisons from the FHIR types, in favor of using the comparisons on the CQL types. Also, it simplifies the public surface of the CQL types:

  • Remove comparison operators from FHIR Date/Time/DateTime/instant
  • Replace Result with normal TryXXXX pattern to align better with normal C# use (unfortunately).
  • Add convenience methods + interface IToSystemPrimitive for consistently converting from a FHIR primitive to a CQL primitive.

@Kasdejong : The last feature will make sure that doing comparisons in FhirPath is a simple IToSystemPrimitive.TryConvertToSystemType() plus + TryCompare(). We should refactor that in the FP engine.

Still Todo:

  • I discovered we have not moved Ratio to Base (which we should) - this would also implement IToSystemPrimitive.
  • We now have TryToSystemXXX() and ToSystemXXX() on some types, and just ToSystemXXX() on others. Unify.
  • Some types have/had a nullable return for ToSystemXXXX(), which would return null if the Value of the primitive was null. I have now removed that, but double check that that makes sense. Esp with TryXXX: would you return false if you would return null if there is no value, or is that allowed?
  • Many of these operations don't have doccomments, need to add that.
  • Unit tests

Fixes #2936, also fixes #2932.

@ewoutkramer
Copy link
Member Author

On nullability of ToSytemXXX(), I think we should align with #2990, so throw if the value is null. In the longer term this is also the proposed solution for other "illegal" values in the primitives. In that case Value will throw, and probably so should all the Toxxxx() functions. This means the datatype contains illegal/incomplete information, which is possible since the datatypes also participate in "overflow" (for now, using ObjectValue) and can contain illegal information. Rather than allowing a user to inadvertedly continue working with that information, we should throw when he/she tries to "do" something with it.

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.

1 participant