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
| error: if manifest.xslx does not exist, then log and fail | ||
| """ | ||
| manifest_path = os.path.sep.join([batch_path,'manifest.xlsx']) | ||
| if not os.path.isfile(manifest_path): |
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
| with open(batch_path+"/manifest.csv", "w") as f: | ||
| c = csv.writer(f) | ||
| for row in sheet.rows: | ||
| sheet_value_arr.append([cell.value for cell in row]) |
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,
There was a problem hiding this comment.
@rzhang152 - nice catch, not sure what "huge data" might look like in this scenario but it would not hurt to add the try/catch @ojas-uls-dev.
There was a problem hiding this comment.
I didn't see the values of sheet_value_arr being used anywhere outside the function, and since the data already exists within the dataframe, so the array seemed redundant. b4259cd was my commit to handle it
bdgregg
left a comment
bdgregg
left a comment
There was a problem hiding this comment.
@ojas-uls-dev - I believe this all looks ok to me.
4 participants
Merging google sheets logic