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
Use XlsxWriter instead of xlwt in to_xls_io() #2592
Conversation
XLSX is now used instead of XLS when deploying forms to KoBoCAT and downloading forms as XLSForm. Fixes #2591.
cb68444
to
5dcdce3
Compare
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.
LGTM but I have some suggestions. Nothing mandatory.
@@ -72,6 +72,7 @@ unicodecsv | |||
uWSGI | |||
xlrd | |||
xlwt |
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.
Would we like to replace xlwt
everywhere?
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.
Maybe. I left xlwt in sync_kobocat_xforms
because I thought that maybe xlwt is faster than XlsxWriter, and that XLS files are generally smaller than XLSX—but I haven't tested my assumptions.
Probably what would be better is to convert directly from CSV to KPI asset JSON instead of using XLS(X) at all:
kpi/kpi/management/commands/sync_kobocat_xforms.py
Lines 169 to 175 in 5dcdce3
# Convert the xlsform to KPI JSON | |
xls_io = io.BytesIO(response.content) | |
if xform.xls.name.endswith('.csv'): | |
dict_repr = xls2json_backends.csv_to_dict(xls_io) | |
xls_io = _convert_dict_to_xls(dict_repr) | |
asset_content = _xlsform_to_kpi_content_schema(xls_io) | |
return asset_content |
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.
Indeed.
Let's keep this for later then.
self.get_object().uid, | ||
request.accepted_renderer.format | ||
) | ||
] = 'attachment; filename={}'.format(filename) |
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.
What about f-string
?
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 agree that it'd be better, but I see myself having to backport this to Python 2 😐
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.
As the above comment. Let's keep the change for later (after python2 support drop).
PS: I won't repeat this 4 times :P
filename = '.'.join((self.get_object().uid, 'xlsx')) | ||
response[ | ||
'Content-Disposition' | ||
] = 'attachment; filename={}'.format(filename) |
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.
What about f-string
?
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 agree that it'd be better, but I see myself having to backport this to Python 2 😐
filename = '.'.join( | ||
(self.get_object().uid, request.accepted_renderer.format) | ||
) |
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.
What about f-string
?
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 agree that it'd be better, but I see myself having to backport this to Python 2 😐
if isinstance(request.accepted_renderer, XlsRenderer): | ||
# `accepted_renderer.format` is 'xls' here for historical | ||
# reasons, but what we actually serve now is xlsx (Excel 2007+) | ||
filename = '.'.join((self.get_object().uid, 'xlsx')) |
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.
What about f-string
?
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 agree that it'd be better, but I see myself having to backport this to Python 2 😐
XLSX is now used instead of XLS when deploying forms to KoBoCAT and downloading forms as XLSForm. Fixes #2591.