Closed sarahmcdougall closed 1 year ago
St.:grey_question: |
Category | Percentage | Covered / Total |
---|---|---|---|
π’ | Statements | 85.26% (-0% π») |
1995/2340 |
π‘ | Branches | 73.9% (-0.01% π») |
1767/2391 |
π’ | Functions | 87.34% (-0.03% π») |
352/403 |
π’ | Lines | 85.56% (+0.01% πΌ) |
1926/2251 |
371 tests passing in 28 suites.
Report generated by π§ͺjest coverage report action from 32926542c469869596a534c482724e41ef8c026f
- I know you were more focused on
detailedResults
for this task, so it is okay if you don't want to add this but I think you can eliminate a call toextractMeasureFromBundle
incalculateDataRequirements
with the following:
Yeah I saw this too but figured it's out of scope. But it's an easy fix so I'll add it
- Also potentially not in the scope of the task and I didn't look into it too much, but I think it may be possible to make the constructor in
MeasureReportBuilder
takemeasure
rather thanMeasureBundle
. I think this would eliminate a call toextractMeasureFromBundle
incalculateGapsInCare
.
Also saw this and figured it's out of scope for now. I don't want to mess with anything related to measure reports until we get detailed results working for composite measures, and it will cause a good deal of unit test fixes.
Summary
Code refactors related to measure extraction for the
calculate
function and its helpers.New behavior
Main motivation: Future work related to composite measure calculation will require looping over component measures and calling various helpers on each component measure. Therefore, we want to limit the amount of code duplication related to measure extraction since we will want to specify the measure outside the helper functions when looping over the components rather than doing a
.find()
for the first measure in the given bundle every time.No new behavior should be introduced.
Code changes
extractMeasurementPeriod
to take in a measure rather than a bundleexecute
to take in a measure in addition to a bundleextractLibrariesFromMeasureBundle
to take in an optional measure in addition to a bundleSome context behind these changes:
extractMeasurementPeriod
is pretty self-explanatory - there's no reason to duplicate the measure extraction withincalculate
and withinextractMeasurementPeriod
execute
comes from the overarching motivation for composite measure calculation. We still need to pass in the entire measure bundle in order to resolve ValueSets. However, we want to specify the measure being passed in rather than extracting it from the bundle once we are in theexecute
function. This will allow us to callexecute
on each component (or on the single measure in the non-composite case).extractLibrariesFromMeasureBundle
is a little more nuanced. We call this function fromretrieveELMInfo
, which itself is called fromexecute
. Therefore, when calculating detailed results, we will want to specify the measure that we are extracting the libraries from (following the same idea of looping over components), and this measure is already passed in toretrieveELMInfo
. However, theextractLibrariesFromMeasureBundle
function is also used forcalculateDataRequirements
andcalculateQueryInfo
, where we don't really need to specify the measure beforehand (at least not within the scope of this task since we are only focusing on composite measure calculation for detailed results). Therefore,measure
is an optional input and will be extracted from the passed-in bundle if it is not supplied as a separate input (open to thoughts on this)extractMeasureFromBundle
functionality is not necessarily what we will want to use with composite measures (since it does a.find()
to get the first measure), I kept the existing functionality for now - figured it was out of scope to change this logic without adding full support for composite measuresTesting guidance
detailedResults
CLI commands to check that calculation behavior has not changedcalculate
to see if there are any other useful refactors we can make now to allow for more flexibility with measure calculation