-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
Improve number parsing and handle a few more events #193
base: master
Are you sure you want to change the base?
Conversation
479edfe
to
a77e2db
Compare
pytr/event.py
Outdated
else: | ||
locales = ("de", "en") | ||
# Prefer german locale. | ||
locales = ("de", "en") |
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.
This will introduce a new issue, sometimes the TR API delivers numbers in American format, this will result in errors such as:
10.002 getting parsed as 10002 instead of 10.002 when parsed by german first.
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.
ok, can you name specific events where that would happen? As I said, at least in my TR account I did not encounter a problem. But it might be just a coincidence.
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 put some more effort into this and checked all the events in my timeline where en
should be preferred before de
when parsing numbers. I also added logs/warnings and an option to dump more data that could help developers.
65f1b99
to
fbd11ba
Compare
fbd11ba
to
9c608a0
Compare
if dump_enabled(): | ||
dump(f"Unknown event {eventTypeStr}: {pprint.pformat(event_dict, indent=4)}") |
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 think this could be handled with logging
just as well, don't you think? And formatting the event_dict
as JSON would make it easier to handle, for example to include as sample data in tests.
if dump_enabled(): | |
dump(f"Unknown event {eventTypeStr}: {pprint.pformat(event_dict, indent=4)}") | |
self._log.debug("Unknown event %r: %s", eventTypeStr, json.dumps(event_dict, indent=4))) |
Where the --dump-debug-data-to-file
option is handled, we can add a FileHandler
to the root logger.
handler = logging.FileHandler(args.dump_debug_data_to_file)
handler.setLevel(logging.DEBUG)
handler.setFormatter(logging.Formatter('%(asctime)s - %(name)s - %(levelname)s - %(message)s'))
logging.root.addHandler(handler)
And maybe shorten the option name to --debug-logfile
?
Alternatively, we could forego the entire option and add a --debug
or --verbose
option to set the log level to DEBUG
such that these messages show up in the console and people can grab it from there; or alternatively have a --debug --logfile myfile.log
as a combination to achieve the same effect.
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.
OK, I'll mull about this. The reason for the if dump_enabled():
construct is that always pretty printing (or json formatting) the data is quite expensive.
This tries to address a few things.
First of all I added two additional event types that occured in my depot lately:
trading_savingsplan_executed
which seems to be something likeSAVINGS_PLAN_EXECUTED
orSAVINGS_PLAN_INVOICE_CREATED
. And additionallyTRADE_CORRECTED
which is likeTRADE_INVOICE
.In order to identify new events in the future I thought it's a good idea to log a warning when unknown events occur. I added code for that.
Furthermore I try to address issue with locales and number parsing, which still seem to exist as per the recent discussion in #141 (#141 (comment)). My idea is to give the german local a general preference before english. With that, at least in my account, going through the whole history, I could not spot regressions. However, if anybody observes something, please let me know.
Generally, most numbers will be parsed correctly because it can be something like
1.234,56
- german which will fail to be parsed as english and1,234.56
which will fail with the german locale. An issue would now be a situation where you have a full english localized in the range of 1000 - 999999 that could be written like1,000
and would now be parsed as 1.0 instead of 1000.0.But as the number format in TR data seems to be wildly mixed it is really hard to predict. Maybe we could change the locale preference for some specific types of events + titles - feedback welcome.