Conversation
jafeltra
left a comment
There was a problem hiding this comment.
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.
mgramigna
left a comment
There was a problem hiding this comment.
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?
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