CSV: correctly serialize booleans and dates. #3841
Conversation
| @@ -322,41 +319,6 @@ def store_result(cls, org, data_source, query_hash, query, data, run_time, retri | |||
| def groups(self): | |||
| return self.data_source.groups | |||
|
|
|||
| def make_csv_content(self): | |||
There was a problem hiding this comment.
Used the opportunity to move this serialization logic from redash.models to redash.serializers.
redash/serializers/query_result.py
Outdated
| query_data = json_loads(query_result.data) | ||
|
|
||
| fieldnames = [] | ||
| bool_columns = [] |
There was a problem hiding this comment.
This feels a tad too scripty for me. I'd prefer to have less details and noise here.
Perhaps this function could be split to several chunks?
- Calling a function to slice
query_data['columns']to three lists:
bool_columns, date_columns, datetime_columns = split_columns_by_type(query_data['columns'])- Handling
csv.DictWriterstuff and for each row:
2a. Calling a function to format booleans
3b. Calling a function to format dates
4c. Calling a function to format date times
There was a problem hiding this comment.
I've started with implementing 1 (5e0ccb9), but then when I tried to split it further I realized I will need to pass around the lists 🤢or convert this into a class 🤔. So ended up doing this instead: feda3c9.
I didn't do this initially as it felt too much, but it does clean the code and makes it easy to add support for new types later.
redash/serializers/query_result.py
Outdated
|
|
||
| for col in query_data['columns']: | ||
| fieldnames.append(col['name']) | ||
| if col['type'] == TYPE_BOOLEAN: |
There was a problem hiding this comment.
These feel pretty mutually exclusive, so an elif might be in place.
| ] | ||
| } | ||
|
|
||
| def get_csv_content(factory): |
There was a problem hiding this comment.
Why not stuff this inside CsvSerializationTest and avoid the passing of factory?
There was a problem hiding this comment.
No good reason :) I just didn't realize at first it will need the factory, and then I moved it out it was easier to pass it the factory -> c6ff138.
| @@ -322,41 +319,6 @@ def store_result(cls, org, data_source, query_hash, query, data, run_time, retri | |||
| def groups(self): | |||
| return self.data_source.groups | |||
|
|
|||
| def make_csv_content(self): | |||
* CSV: correctly serialize booleans and dates. Closes getredash#3736, closes getredash#2751. * pep8 fixes * Move column iteration to a helper function. * Use elif, as types are mutually exclusive. * Refactor parsing implementation. * Move the csv generation fucntion
What type of PR is this? (check all applicable)
Description
When downloading CSV version of a query result, it will:
true/false(instead ofTrue/False).Related Tickets & Documents
Closes #3736, closes #2751.