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

Refactor to separate gaps and data requirements #293

Merged
merged 5 commits into from
Feb 12, 2024
Merged

Conversation

lmd59
Copy link
Contributor

@lmd59 lmd59 commented Jan 29, 2024

Summary

Consolidate helpers, rename files as helpful, and copy functionality or move to shared helpers so that gaps and dataRequirements can function independently without cross referencing each other.

New behavior

None

Code changes

  • Moved QueryFilterParser and RetrievesHelper to elm helpers
  • Moved ClauseResultsHelper, rename ReportBuilderFactory
  • Moved GapsReportBuilder to calculation and rename import to match file name, rename test to GapsReportBuilder.test
  • Remove GapsReportBuilder dependency from DataRequirementHelpers and replicate dependeny function
  • Moved detailedFilter functions and codeLookup and flattenFilters to QueryFilterParser
  • Test and other import updates for all of these

Testing guidance

  • npm run check
  • Run integration tests
  • Test other basic functionality by running gaps and dataRequirements on your favorite test measure, test patient, and test library

Copy link

github-actions bot commented Jan 29, 2024

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
85.51% (-0.87% 🔻)
2373/2775
🟡 Branches
73.26% (-0.51% 🔻)
2208/3014
🟢 Functions
88.15% (-0.74% 🔻)
424/481
🟢 Lines
85.85% (-0.88% 🔻)
2293/2671
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / GapsReportBuilder.ts
93.66% 83.91% 100% 93.63%
🟢
... / QueryFilterParser.ts
86.69% 80.93% 97.62% 86.47%
🟡
... / ClauseResultsHelpers.ts
78.1% 87.86% 84.62% 77.78%
🟢
... / RetrievesHelper.ts
97.53% 84.69% 100% 97.37%
🟢
... / ReportBuilderFactory.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / DataRequirementHelpers.ts
66.26% (-18.77% 🔻)
64.81% (-15.5% 🔻)
58.33% (-20.24% 🔻)
67.09% (-18.63% 🔻)

Test suite run success

451 tests passing in 31 suites.

Report generated by 🧪jest coverage report action from caf1668

@elsaperelli elsaperelli self-requested a review January 31, 2024 21:18
@elsaperelli elsaperelli self-assigned this Jan 31, 2024
Copy link
Contributor

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still in the process of reviewing this PR, but I did notice that the regression script (command: ./regression/run-regression.sh -b master -v) fails and I don't think it should. At first glance, I suspect it has to do with simply the movement of files, but I will look into it more.

Copy link
Contributor

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woo refactoring! A tough thing to review but very necessary changes. I am hesitant about changing calculateDataRequirements name at the moment so I want to see what you think. I also think it's possible that there's a duplicate function but I am not sure.

src/cli.ts Outdated Show resolved Hide resolved
src/helpers/DataRequirementHelpers.ts Show resolved Hide resolved
Copy link
Contributor

@elsaperelli elsaperelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Contributor

@hossenlopp hossenlopp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of moving things around. But this does look good.

@hossenlopp hossenlopp merged commit 4f04586 into master Feb 12, 2024
5 checks passed
@hossenlopp hossenlopp deleted the dr-refactor branch February 12, 2024 22:17
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.

3 participants