-
Couldn't load subscription status.
- Fork 76
[ENG-8475] | fix unable to render spreadsheet files (.xls and .xlsx) #393
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
[ENG-8475] | fix unable to render spreadsheet files (.xls and .xlsx) #393
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.
Looks good to me. I didn't delve too deep into to_bytes, parse_xls or parse_xlsx and let's test them after merge. Make sure you have different test files to test different conditions. We can have a new ticket/PR to add unit tests if needed.
| • .xls → xlrd | ||
| • .xlsx → openpyxl (xlrd ≥2.0 dropped xlsx support) | ||
| `fp` is the stream returned by WaterButler/MFR. It may already have been | ||
| read, so we always rewind and copy to an in‑memory buffer that openpyxl (and | ||
| ZipFile) can seek inside safely. |
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.
👍 for the Docstrings
| wb = xlrd.open_workbook(file_contents=to_bytes(fp)) | ||
| return parse_xls(wb, sheets) | ||
| except xlrd.biffh.XLRDError: | ||
| pass |
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.
I see. We give it a try with xlrd. If failed, we try with openpyxl. I think reasons other than xlsx can cause xlrd.biffh.XLRDError but I think it's fine.
| raise xlrd.biffh.XLRDError( | ||
| "Invalid xlsx file or corrupted ZIP structure" | ||
| ) from exc |
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.
Yep, this answered my previous questions.
d4da5a8
into
CenterForOpenScience:feature/buff-worms
Ticket
Purpose
Changes
Side effects
QA Notes
Deployment Notes