Refactor event exporter and add JSON output format #189
Conversation
…n to 1/3rd of terminal width but capped at 80 for a cleaner help output
…with-time` and `--[no-]decimal-localization`
|
Overall, looks like a good refactoring/enhancement. Please give me some time to play with it. |
| transactions = (txn for event in events for txn in self.from_event(event)) | ||
|
|
||
| if format == "csv": | ||
| writer = csv.DictWriter(fp, fieldnames=self.fields(), delimiter=self.csv_delimiter) |
There was a problem hiding this comment.
| writer = csv.DictWriter(fp, fieldnames=self.fields(), delimiter=self.csv_delimiter) | |
| lineterminator = "\n" if platform.system() == "Windows" else "\r\n" | |
| writer = csv.DictWriter(fp, fieldnames=self.fields(), delimiter=";", lineterminator=lineterminator) |
The delimiter needs to be different on Windows. (Or you open the file with newline="").
There was a problem hiding this comment.
I've found this SO question about the same issue: https://stackoverflow.com/questions/3191528/csv-in-python-adding-an-extra-carriage-return-on-windows
The answer given here seems a more appropriate solution, considering it is also what is recommended in the documentation of the csv module: Opening the file with newline='' to disable universal newline.
There was a problem hiding this comment.
yes, as written above, you could open the file with newline=''. However the output file for exporting transactions is opened through type=argparse.FileType("w", encoding="utf8"), in main.py and this does not support the newline option. But I guess you could refactor that.
There was a problem hiding this comment.
🤦 Sorry for overlooking that you already noted that. You're right of course, argparse.FileType does not accept the argument. People in Issue24739 encourages you to create your own FileType-like custom class that accepts the newline argument 🤔
Co-authored-by: Christoph Langer <[email protected]>
Summary
This pull request refactors the transaction exporter code to improve maintainability and introduces support for exporting transactions in JSON format. This support for JSON sets itself apart from #50 in that it is not the raw Trade Republic timeline transactions that are exported, but instead our own summarized representation of events.
Key changes
csv.DictWriterfor CSV formatting instead of custom code.--[no-]decimal-localizationoption which defaults toFalse. This changes the current default behavior to format decimal numbers in the selected locale.--date-with-isoformatto--[no-]date-with-timeas it is more correct: The previous date-only format was also ISO8601 compatible. This option now defaults toTrue, which is also different from the previous default behavior.Additional notes
Changing the default behavior for CSV export could have some impact on users relying on the precise output format of the CSV, though I think that impact depends largely on how picky the applications are that the CSV gets fed into (and I don't have any context about that).
I'm suggesting this change regardless because I feel it is generally speaking the more useful output format for further processing (with time in the date and no decimal localization). We should probably remove, or change, the note about
export_transactionstargeting specifically "Portfolio Performance".