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

Added datetime support #3

Merged
1 commit merged into from
Oct 27, 2010
Merged

Added datetime support #3

1 commit merged into from
Oct 27, 2010

Conversation

hildjj
Copy link
Contributor

@hildjj hildjj commented Oct 23, 2010

If you assume that the inbound datetime is UTC, it's not so bad.

@martinkou
Copy link

I didn't include datetime support in the beginning because Python's timezone support is a mess. Your patch could work if we do our best to make sure the BSON output is in UTC, and that the deserialized datetime output is also in UTC. If we're unsure about the input or output in either steps, warn the developer. There's no ambiguity as long as the developer is using timezone-aware datetime objects so we should encourage that. I'd suggest a few more changes to this patch...

  1. Instead of using time.mktime() and timetuple() in encode_UTCdatetime_element(), use calendar.timegm() and utctimetuple(). This ensures a timezone-aware datetime object would be properly converted to UTC.
  2. If the incoming datetime object to encode_UTCdatetime_element() has no tzinfo (e.g. the default output of datetime.now() with no tz), we should warn() the developer about the ambiguity and the developer will have to filter out the warning himself if he's sure he's using the datetime objects correctly. We should most probably create our own warning category class for this case.
  3. If possible, I think decode_UTCdatetime_element() should return a UTC zoned datetime object if possible. It's a shame that Python doesn't include its own timezone database. But we can add pytz to the package's dependency list and always return datetime objects zoned to pytz.utc there.

This pull request was closed.
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.

2 participants