-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat: implement GET DIRECT DATA for CSV #82
Conversation
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.
The new fix for resolving relative to model dir looks good, but I just remembered that we still have the issue that we spotted yesterday where readCsv
logs the error instead of throwing.
I think maybe it would be best to remove the try/catch in readCsv
and let errors propagate up. Right now the error message suggests that the operation failed because the file doesn't exist, but it could also be that the CSV data was invalid or something, so removing the try/catch might be sufficient.
Can you either fix that as part of this branch, or open a new issue and address it as a separate PR? Either one is fine by me, but just wanted to make sure that issue didn't get lost.
I removed the try/catch block around the CSV parse. I tested it with a bad pathname and got the following unhandled exception error message:
I think this is clear enough that we don't need a special error message. |
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.
Todd addressed the readCsv
error handling issue I mentioned in my last comment, and that fix has been merged into this branch (via PR #93), so this branch is now ready to be merged. I'll merge it to develop
shortly.
Fixes #81
Add CSV support to GET DIRECT DATA. This required refactoring the existing XLSX support a bit to abstract away data access when converting the data to a lookup. CSV tests were added to the directdata test model.