-
Notifications
You must be signed in to change notification settings - Fork 112
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
Fix crash when parsing date #405
Conversation
Dangit, I didn't realize dateutils wasn't part of the standard library 🤦🏻♀️ |
Thanks for the PR. Can you fix this without introducing a new library dependency? The datetime string you posted, |
When running
I can verify that it does by running
That crashes with the same error message for me. I then found this StackExchange thread which is what suggested dateutil 🤷🏻♀️ |
I'm on Debian GNU/Linux 12 (bookworm). The Debian stable version of toot (which is 0.34) doesn't have this issue but I downloaded the git master version to work on an unrelated issue and then I ran into this. According to that aforementioned thread support for this was added in version 3.11 so people on that version won't be able to replicate the bug. Still a problem for me though 🤷🏻♀️ |
OK thanks for the explanation, seems reasonable to me to add python-dateutil as we have a lot of date parsing that we need to get right. Hopefully @ihabunek will be back soon to weigh in on this. Meanwhile, if you can fix up the imports so the automated tests pass, I think this PR will be acccepted. |
Not sure how to do that 💁🏻♀️ |
Here's a fix that avoids the python-dateutil dependency: rather than calling: And, for good measure, convert any "Z" suffix (if it exists) to +00:00 as per this thread Running with python3 (I'm running 3.10, which doesn't have the 3.11 improvements to
Python-dateutil is more robust, but this should work for us, and we can always change later if necessary. And the above code change should pass the automated tests. |
I can verify that that doesn't crash ♥♥👍🏻👍🏻 |
Solution found by danschwarz
More interesting things... #399 notes a documentation discrepancy in Mastodon regarding this field, and the toot crash re: "controversial timezone workaround" - this was controversial at the time, but it seems that those who wanted to expand Alternately, we could just use
By the way - There was no test case exercising this code path in Here's a patch file that adds the fields to the test cases. If you would git patch your branch with this file and push the changes to the PR, I'd appreciate it. |
Thank you for sending a patch 👍🏻 (instead of a PR on my branch 😵💫)
|
I was getting a
ValueError: Invalid isoformat string: '2023-09-23T13:07:50'
crash.