Skip to content

Conversation

@Dtphelan1
Copy link
Contributor

Pretty straightforward. Should only need one reviewer.

  • Confirm that the client works when you run it with the provided sample data.
  • Confirm that my test changes are adequete and sensible.
  • Provide any feedback on the logged warn message.

@Dtphelan1 Dtphelan1 force-pushed the csv-module-graceful-fail-on-filter branch from ca9558a to cecf525 Compare April 13, 2021 23:20
@jafeltra jafeltra self-assigned this Apr 14, 2021
@jafeltra
Copy link
Contributor

This looks really good! I noticed one thing that might be worth updating in this PR. When running the client with the provided sample data, patient 4 doesn't have a patient information row, which means that the patient resources is not extracted. However, then when we go to mask the patient data, it cannot find a patient resource and an error is thrown and I think extraction stops because no bundles are written.

I also had two quick questions, but they might be things you were looking at separately, in which case both can definitely wait. When I run the client with the provided sample data, the CSVPatientExtractor and the CSV ClinicalTrialInformationExtractor throw other errors about destructuring from undefined. Is handling those cases something you were looking at in the other branch? It also looks like some extractors log their own warning saying "No disease status data found for patient" but not all of them log that and just rely on the new warning in the CSV Module. Is that also expected?

Copy link
Contributor

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

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

Talked to Dylan about the last two questions and they'll be handled in another task. This looks good tome!

@jafeltra jafeltra merged commit 21689a2 into develop Apr 15, 2021
@jafeltra jafeltra deleted the csv-module-graceful-fail-on-filter branch April 15, 2021 14:55
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