-
Notifications
You must be signed in to change notification settings - Fork 145
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
Handle microseconds in Time SchemaType #319
Conversation
colander/__init__.py
Outdated
fmt = { | ||
8:'%H:%M:%S', | ||
5:'%H:%M' | ||
}.get(len(cstruct), '%H:%M:%S.%f') |
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.
Instead of trying to guess based upon length, why not just attempts to go through each one in order? Start with the ms one, followed by the one with seconds, followed by the one with just hours:minutes?
Much like the original solution did, but loop over a list of formats.
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.
Because I thought nested exceptions were not very readable and filled the stack trace for not much. While the proposed solution is equivalent and more readable, in my opinion. But maybe I'm missing something and not understanding your proposed loop over formats using exceptions.
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 would also prefer a solution that looped over the formats and tried them. Finally if none of them worked then raise an exception. You could pass in the final error object as the err
key. I don't think any nested exceptions would occur if this were a loop.
I'm concerned about testing the length because I don't actually know if the %H
etc always maps to two characters - it defers to libc and possibly any locale settings which may cause unexpected regressions. It's safer to just test the formats and use the one that works since the formats are unambiguous.
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.
Loop over the formats instead of relying on length.
Thanks @blqn ! |
Actually, it is impossible to manage microseconds using Time SchemaType:
This PR handle full ISO format HH:MM[:SS[.mmmmmm]] for deserialization and HH:MM:SS[.mmmmmm] for serialization through time.isoformat.