-
Notifications
You must be signed in to change notification settings - Fork 2
Numerous Extractor Updates #108
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
Conversation
jafeltra
left a comment
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.
This looks great to me! I ran it with all the extractors in my config and all the resulting bundles are equivalent to what is on develop, and those few errors about destructuring are resolved. I had one incredibly minor comment in line and just one questions about tests:
Do you think a test that each extractor returns an empty bundle if there are no entries returned from the CSV module would be worthwhile? I'm not sure if it's overkill to test that, but it's the only new functionality that I could think of that isn't already tested.
jafeltra
left a comment
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.
I made a Jira task for the empty bundle tests. This looks great!
mgramigna
left a comment
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.
Can you update the example csv config JSON file to have the patient extractor first so our example would be kosher with context in a hypothetical extraction situation?
mgramigna
left a comment
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.
Looks good!
559b3ad to
98c5b37
Compare
Summary
We don't mention scope creep...So this PR looks like it does a lot more than it actually does. Originally, the goal of this PR covered a few items:This second change very gently suggested two other reasonable (if not sprawling) changes:
MRNproperty in joinAndReformat. The MRN requirement should be a result of the CSVmodule.get, not a result of the reformatData function.Overall, these are very small changes that stretch wide distances. Additionally, since changes take place at such a foundational level (e.g. in some cases the interfaces for generatingMcodeResources has changed) there is a sibling PR in the E-MEF.
Testing:
getroutinemrnbeyond the original CSVModule call