FirelyTeam / firely-cql-sdk

BSD 3-Clause "New" or "Revised" License
31 stars 17 forks source link

570 bug on handling conversion from a choice type #571

Closed baseTwo closed 2 months ago

baseTwo commented 2 months ago

Fix for #570

⚠️ Regarding ChoiceTypeConversionTests I was unable to get the same scenario as the RR23 test from Rene where the choice type correctly uses the ICqlOperators.Convert<T>() method. It is a coincidence that this test is passing. Also, Convert throws exceptions if no conversion is available, instead of the safe type cast as in C#

There are a about 15 tests that failed in CMS measures because of this. For choice types, there should be a TryConvert method that returns a null instead, but this needs to be thought out quite better in 2024 Q4.

Are we rushing this fix through too prematurely?

ewoutkramer commented 2 months ago

Are we rushing this fix through too prematurely? Choice types was not really on the agenda for Q3.

Yes, I think so. Instead of solving this statically we push it to run time, which requires the Firely converters to do the heavy lifting, for which they were not tested or designed yet. When we manage to fix this statically, we might as well remove these capabilities from the run time. That said, it may take a while before we get to implementing choices definitively, as we might want to wait until DU's are implemented natively in .NET. So, we'd have to make do until that time. It will always be patchy, since we don't want to do all the conversions at run time.

Most important for me at this point is that this is not a small fix, and I'd like to have @EvanMachusak's opinion on whether this is something that might break the HEDIS things, in which case it's not worth doing it now. If it does not break HEDIS, I think I am ok with it.

baseTwo commented 2 months ago

I have thought up some other way that we don't need to use the runtime convert. I'll set the PR back to draft.

Basically we can use a switch expression over the choice instance's types, and on each case do the necessary ChangeType in the expression builder to the target type .

Continuing on PR #577