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

Datetime support, implementing issue #86 #89

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@lelit

lelit commented Mar 14, 2014

I think I'm done: all tests pass on Python 2.6, 2.7 and 3.3.

lelit added some commits Mar 13, 2014

Handle serialization and recognition of Python datetime, date and time
A new boolean option, `handle_datetime`, activates the serialization and
deserialization of Python datetime, date and time instances, using their
standard ``isoformat()`` method.

Only `naive` instances are supported, both from the pure Python
implementation and from the C speedups.
Remove Python 3 only code within a Python 2 only block
The whole function is surrounded by a ``#if PY_MAJOR_VERSION < 3``
so there is no point keeping ``#if (PY_MAJOR_VERSION >= 3`` blocks
in its code.
Fix parse of times with only milliseconds in the Python impl
Also, recognize also a space as separator between the date and the
time, for consistency with the C implementation.
Comply with Bob's requirements expressed in issue #86
* rename the flag to activate the new behaviour from "handle_datetime"
  to a more clear "iso_datetime"

* serialize datetimes and times with an ending "Z" to make it explicit
  that they are naive values; the parser does recognize either forms
  though

Correct also the Python 3 variant of the C speedups to properly handle
the different FSR internal representations.
@etrepum

This comment has been minimized.

Show comment
Hide comment
@etrepum

etrepum Mar 14, 2014

"0001-12-25T10:20:30.123456Z" is not the expected output (unless your local time is UTC). The expected output should be in UTC, which is what the Z means.

etrepum commented on index.rst in dc2bea2 Mar 14, 2014

"0001-12-25T10:20:30.123456Z" is not the expected output (unless your local time is UTC). The expected output should be in UTC, which is what the Z means.

@etrepum

This comment has been minimized.

Show comment
Hide comment
@etrepum

etrepum Mar 14, 2014

I don't think that supporting time() is within scope here. The idea here is to have parity with Javascript's JSON implementation. Javascript doesn't have a time type.

etrepum commented on index.rst in dc2bea2 Mar 14, 2014

I don't think that supporting time() is within scope here. The idea here is to have parity with Javascript's JSON implementation. Javascript doesn't have a time type.

@etrepum

This comment has been minimized.

Show comment
Hide comment
@etrepum

etrepum Mar 14, 2014

I never specified anything about decoding. This pull request should not change the behavior of load/loads at all. Support for datetime should be one way.

etrepum commented on index.rst in dc2bea2 Mar 14, 2014

I never specified anything about decoding. This pull request should not change the behavior of load/loads at all. Support for datetime should be one way.

@etrepum

This comment has been minimized.

Show comment
Hide comment
@etrepum

etrepum Mar 14, 2014

Member

This looks like a great start but it's not what we discussed on the other ticket, there should be no parsing code in here and timestamps without a time zone should be converted to UTC first.

Member

etrepum commented Mar 14, 2014

This looks like a great start but it's not what we discussed on the other ticket, there should be no parsing code in here and timestamps without a time zone should be converted to UTC first.

@lelit

This comment has been minimized.

Show comment
Hide comment
@lelit

lelit Mar 14, 2014

I'm afraid I do not understand the first comment about "local time": only naive datetime are exported, and by definition those are without a timezone... that sample output is just that, what does it make you think it's not UTC?

On the other comments, well, given the reactions this feature raised, I'm leading toward making my own fork, because it's actually the loading part that's the most interesting for me, the one that written in Python is rather slow.

Since both ways are opt-in, I do not understand why one should have one or the other, but not both. If you don't specify "iso_datetime" on dumps(), nothing new happens. If you don't specify "iso_datetime" on loads(), nothing new happens neither.

lelit commented Mar 14, 2014

I'm afraid I do not understand the first comment about "local time": only naive datetime are exported, and by definition those are without a timezone... that sample output is just that, what does it make you think it's not UTC?

On the other comments, well, given the reactions this feature raised, I'm leading toward making my own fork, because it's actually the loading part that's the most interesting for me, the one that written in Python is rather slow.

Since both ways are opt-in, I do not understand why one should have one or the other, but not both. If you don't specify "iso_datetime" on dumps(), nothing new happens. If you don't specify "iso_datetime" on loads(), nothing new happens neither.

@etrepum

This comment has been minimized.

Show comment
Hide comment
@etrepum

etrepum Mar 14, 2014

Member

Naive datetime should be assumed to be in the local timezone, which should be converted to UTC before serialization. It makes me think that it's not UTC because the roundtrip on that test should fail in any environment that isn't set to UTC. Looking at other parts of the code, it looks like this kind of incorrect assumption lies elsewhere. The trailing 'Z' means UTC, not naive timestamp. It's shorthand for "-00:00" or "+00:00". http://www.w3.org/TR/NOTE-datetime

Having your own fork is fine, I'm not likely to accept any patch that adds parsing support for datetime in this manner, and it's not something you can reasonably implement (in the parse phase) without modifying the library. I'll go ahead and close this.

Member

etrepum commented Mar 14, 2014

Naive datetime should be assumed to be in the local timezone, which should be converted to UTC before serialization. It makes me think that it's not UTC because the roundtrip on that test should fail in any environment that isn't set to UTC. Looking at other parts of the code, it looks like this kind of incorrect assumption lies elsewhere. The trailing 'Z' means UTC, not naive timestamp. It's shorthand for "-00:00" or "+00:00". http://www.w3.org/TR/NOTE-datetime

Having your own fork is fine, I'm not likely to accept any patch that adds parsing support for datetime in this manner, and it's not something you can reasonably implement (in the parse phase) without modifying the library. I'll go ahead and close this.

@etrepum etrepum closed this Mar 14, 2014

@lelit

This comment has been minimized.

Show comment
Hide comment
@lelit

lelit Mar 14, 2014

Ok, thanks anyway for taking time to review.

lelit commented Mar 14, 2014

Ok, thanks anyway for taking time to review.

@etrepum

This comment has been minimized.

Show comment
Hide comment
@etrepum

etrepum Mar 14, 2014

Member

I didn't give it a thorough review, just the documentation changes at top of that one commit. I stopped before getting to any of the actual code. You should have someone else take a closer look if you intend to use your fork in production.

Member

etrepum commented Mar 14, 2014

I didn't give it a thorough review, just the documentation changes at top of that one commit. I stopped before getting to any of the actual code. You should have someone else take a closer look if you intend to use your fork in production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment