Skip to content
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

fix: handle XLSX files in GET DIRECT {CONSTANTS,DATA,LOOKUPS} #400

Merged
merged 10 commits into from
Nov 15, 2023

Conversation

chrispcampbell
Copy link
Contributor

Fixes #399

See issue for more details.

I basically consolidated the handling of the 3 different kinds of tabular file sources:

  • named csv file
  • named xls[x] file
  • tagged xls[x] file

And now any of those 3 sources work with any of:

  • GET DIRECT CONSTANTS
  • GET DIRECT DATA
  • GET DIRECT LOOKUPS

Plenty of test coverage on this one.

Copy link
Collaborator

@ToddFincannonEI ToddFincannonEI left a comment

Choose a reason for hiding this comment

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

I'm glad to see this cleaned up. The only potential problem I saw was in the handleExcelOrCsvFile function, where the file extension is checked against lower case "csv". This would fail if the file name was upper case. I checked path.resolve and confirmed it does not normalize case.

@chrispcampbell
Copy link
Contributor Author

I'm glad to see this cleaned up. The only potential problem I saw was in the handleExcelOrCsvFile function, where the file extension is checked against lower case "csv". This would fail if the file name was upper case. I checked path.resolve and confirmed it does not normalize case.

Good catch. I updated the fix so that it converts the path to lowercase before checking the extension. Also renamed one of the test files to have an uppercase CSV extension to verify this. Will merge shortly.

@chrispcampbell chrispcampbell merged commit 16b1ddf into main Nov 15, 2023
6 checks passed
@chrispcampbell chrispcampbell deleted the chris/399-get-direct-xlsx branch November 15, 2023 22:28
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.

Handle XLSX files in GET DIRECT {CONSTANTS,DATA,LOOKUPS}
2 participants