Support CSVs with blank lines and empty values #169
Conversation
Dtphelan1
requested review from
dmendelowitz,
jafeltra,
julianxcarter and
mgramigna
as code owners
dmendelowitz
left a comment
dmendelowitz
left a comment
There was a problem hiding this comment.
This looks great! Just had one small comment
|
|
||
| const INVALID_MRN = 'INVALID MRN'; | ||
| const csvFileModule = new CSVFileModule(path.join(__dirname, './fixtures/example-csv.csv')); | ||
| const csvFileModuleWithBOMs = new CSVFileModule(path.join(__dirname, './fixtures/example-csv-bom.csv')); |
There was a problem hiding this comment.
I think we can delete example-csv-bom.csv now, right? It's not used anywhere else.
There was a problem hiding this comment.
We are still using it, just encapsulated it within the test that uses it instead of leaving it up at the top
There was a problem hiding this comment.
Sorry if I'm missing something obvious but I don't see the file used anywhere in the tests. The BOM test uses example-csv-empty-values.csv, is that intentional? I think that file has a BOM so it is an accurate test
There was a problem hiding this comment.
Definitely not intentional. Definitely a typo. Fixing now.
|
I'm not officially reviewing this PR, but when I was looking through the code yesterday and when Dylan M mentioned the bom nightmares, I remembered there are two other places we probably need these option as well. There's the |
I wonder if it would be worth it to write a wrapper function that just calls |
Ah that's a good point. I feel like that could definitely be worthwhile. |
Summary
Supports ingestion of CSV's with either totally blank rows of data or rows with nothing by empty-values; in both cases those rows will be ignored.
New behavior
Now the CSVFileModule can ingest files with rows that meet the above criteria.
Code changes
Some new tests
Testing guidance
Make sure the tool still works.