Conversation
ojas-uls-dev
requested review from
alex-wreschnig,
bdgregg,
chryslovelace,
ctgraham and
rzhang152
|
see also issue #1 |
ctgraham
left a comment
ctgraham
left a comment
There was a problem hiding this comment.
Some questions and some change requests.
…ment, when before it was inherited from parent namespace
Remove specific file patterns from .gitignore.
Updated path handling to use os.path.sep for cross-platform compatibility.
Removed commented-out print statement and condition check related to debugging.
…sing into main function. Added a --log-file option.
…pandas call works better.
|
Tested the script to use either a spreadsheet or Google Sheet. currently the script functions as expected. |
ojas-uls-dev
left a comment
ojas-uls-dev
left a comment
There was a problem hiding this comment.
Aside from what clinton mentioned about the erroneous print statement, it looks correct
make-batch-dirs
Outdated
There was a problem hiding this comment.
i see the difference between docs and logic, so I'll fix that. It seems like the code is copying file provided by args.xls_file to manifest_path i.e. batch_path/manifest.xslx. This should work fine if args.xls_file exists but manifest.xsl does not exist
make-batch-dirs
Outdated
There was a problem hiding this comment.
- append() will keep allocating more memory, so it might risk a MemoryError if the sheet contain huge data. If sheet_value_arr is not referred in other place, might directly stream rows from openpyxl to CSV.
2 writer.writerow() could be failed, it might be betterto wrap the overall workflow in try/except,
| logger.error(f"Error: Google arguments are required when using Google Sheets.") | ||
| exit() | ||
| # Else we are using a Spreadsheet... | ||
| else: |
There was a problem hiding this comment.
I think we can add some error checking around if args.xls_file exists or not before trying to read the xlsx file.
4 participants
Merging google sheets logic