rapidpro / rapidpro-python

Python client library for the RapidPro API
BSD 3-Clause "New" or "Revised" License
18 stars 27 forks source link

DatetimeField serialisation inconsistent with format used in rapidpro and ISO 8601 #39

Closed system7ltd closed 7 years ago

system7ltd commented 8 years ago
  1. The format_iso8601 function does not fully conform to ISO 8601. The datetime object is converted to UTC before serialisation but no time zone designator is added to the reformatted string. Rapidpro does include time zone designators.
  2. By default, rapidpro does not include microseconds in the serialised datetime.

Ideally, serialize() should be an inverse function of deserialize() - i.e. after deserialising data received from rapidpro, rapidpro-python should yield identical output to the data originally received when the programmer uses rapidpro-python to (re)serialise that data.

rowanseymour commented 8 years ago

Hey @system7ltd. We should fix the the lack of timezone designator, but the use of microseconds is intentional. RapidPro's API v1 will accept such datetimes, and API v2 uses them consistently.

Can I ask what you're doing with serialization of returned objects?

system7ltd commented 8 years ago

Hey @rowanseymour, thank you for your reply. I see, I had noticed the problem after getting Runs with v1 - rapidpro.io returns all datetime fields of the FlowRun objects with millisecond precision. I then glanced at the rapidpro source code and saw the following:

def format_datetime(value):
    """
    Datetime fields are limited to millisecond accuracy for v1
    """

class DateTimeField(serializers.DateTimeField):
    """
    For backward compatibility, datetime fields are limited to millisecond accuracy
    """

def datetime_to_json_date(dt, micros=False):
    """
    Formats a datetime as a string for inclusion in JSON
    :param dt: the datetime to format
    :param micros: whether to include microseconds
    """

That made me assume that rapidpro had established a standard to which all datetime fields should conform. After reading your reply, I checked other objects and can see that there was indeed no consistency in v1 (e.g. unlike runs, steps are serialised with microsecond precision) and v2 seems to have switched to microseconds for Runs (and everything else, I assume), too. I have just updated this PR to deal only with the time zone designator. I hope this helps.

To answer your question, I thought about using rapidpro-python as a proper way to connect to rapidpro.io to get the data I need (instead of just manually getting that as JSON over HTTP) so I had written some tests and stumbled upon this and some other issues.

rowanseymour commented 8 years ago

v2 should use the same format everywhere (microseconds + Z) and if you find a place where it isn't then that's a bug.

We definitely recommend using this API client library if you're doing anything in Python - we use it for all our dashboard projects and will keep it up to date with any changes moving forward.

I'm still not getting what you need serialization for (as opposed to de-serialization).

system7ltd commented 7 years ago

Hey @rowanseymour, thank you for your reply and reviewing my modifications. I was playing with serialisation and exploring rapidpro-python while I worked on a little project in my spare time (https://pypi.python.org/pypi/rapidpro-pull) and just thought you would like to have the problems I found reported and fixed in the true spirit of open source (thank you for your contributions to the Free/Libre Software community BTW :)!) - hence this and the other thread and my pull requests. This issue has now been fixed so let us close it. See you around and Merry Christmas! :)

rowanseymour commented 7 years ago

@system7ltd same to you and feel free to report or fix any other issues you run into