Skip to content
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

Merged
merged 2 commits into from
Jan 31, 2019
Merged

Conversation

blqn
Copy link

@blqn blqn commented Jan 24, 2019

Actually, it is impossible to manage microseconds using Time SchemaType:

>>> from colander import Time
>>> from colander import SchemaNode
>>> from datetime import time
>>> node = SchemaNode(Time())
>>> node.deserialize('10:42:42.424242')
...
colander.Invalid: {'': 'Invalid time'}
>>> t = time(10,42,42,424242)
>>> node.serialize(t)
'10:42:42'

This PR handle full ISO format HH:MM[:SS[.mmmmmm]] for deserialization and HH:MM:SS[.mmmmmm] for serialization through time.isoformat.

fmt = {
8:'%H:%M:%S',
5:'%H:%M'
}.get(len(cstruct), '%H:%M:%S.%f')
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

Copy link
Member

@mmerickel mmerickel left a 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.

@mmerickel mmerickel merged commit 801a367 into Pylons:master Jan 31, 2019
mmerickel added a commit that referenced this pull request Jan 31, 2019
@mmerickel
Copy link
Member

Thanks @blqn !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants