-
Notifications
You must be signed in to change notification settings - Fork 38
Capture warnings as well as errors from CQL evaluation results. #840
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
base: master
Are you sure you want to change the base?
Conversation
lukedegruchy
commented
Nov 24, 2025
- Capture warnings as well as errors from CQL evaluation results.
|
Formatting check succeeded! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #840 +/- ##
============================================
- Coverage 73.68% 73.67% -0.01%
Complexity 277 277
============================================
Files 576 576
Lines 27159 27183 +24
Branches 3459 3462 +3
============================================
+ Hits 20013 20028 +15
- Misses 5431 5438 +7
- Partials 1715 1717 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| * When a warning or info is produced while evaluating, we capture the errors generated in this object, which can be rendered per Measure evaluated. | ||
| * @return {@code Map<IIdType, List<String>>} | ||
| */ | ||
| public Map<MeasureDef, List<String>> getWarningsPerMeasure() { |
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.
The documentation says this is capturing warnings or infos, but that distinction is not preserved, is this intentional? Should there be a way to indicate warning vs info?
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.
Not without CQL changes. I'm fine to go ahead and do that, but what's the use case?
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.
Do you mean CQL engine changes, or CQL content changes? We're reworking the engine APIs now, so if you mean that this would be good feedback. :)
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.
What Bryn is asking for is not possible without CQL changes to pass down infos separately to the multi-lib CQL evaluation result class.
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.
Right, so can we do that? It would be wrong (imo) to return as a "warning" something that was an "info", and vice versa.
| // We may get several errors for a given measure | ||
| private final Map<MeasureDef, List<String>> errorsPerMeasure; | ||
| // We may get several warnings for a given measure | ||
| private final Map<MeasureDef, List<String>> warningsPerMeasure; |
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.
There can be more information coming back from a message in CQL than just the message, it may have additional information, should we also expose that information?
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.
We don't expose it for errors either, and this will require CQL changes. What are you looking to capture?
...c/main/java/org/opencds/cqf/fhir/cr/measure/common/CompositeEvaluationResultsPerMeasure.java
Outdated
Show resolved
Hide resolved
| * When a warning or info is produced while evaluating, we capture the errors generated in this object, which can be rendered per Measure evaluated. | ||
| * @return {@code Map<IIdType, List<String>>} | ||
| */ | ||
| public Map<MeasureDef, List<String>> getWarningsPerMeasure() { |
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.
Right, so can we do that? It would be wrong (imo) to return as a "warning" something that was an "info", and vice versa.