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

TimeDelta field bug #105

Closed
philtay opened this Issue Dec 14, 2014 · 8 comments

Comments

Projects
None yet
2 participants
@philtay
Contributor

philtay commented Dec 14, 2014

Float strikes again:

from decimal import Decimal as D
from datetime import timedelta as td
from marshmallow.compat import total_seconds
from marshmallow.fields import TimeDelta

a = td(microseconds=1)
D(total_seconds(a))

# Decimal('9.99999999999999954748111825886258685613938723690807819366455078125E-7')

a = TimeDelta()
b = a.deserialize(0.1)
c = total_seconds(b)
D(c)

# Decimal('0.1000000000000000055511151231257827021181583404541015625')
@philtay

This comment has been minimized.

Contributor

philtay commented Dec 14, 2014

IMO the solution is to use integer instead of float and drop microseconds support. Here the standard to stick to is ISO 8601 (durations).

http://en.wikipedia.org/wiki/ISO_8601#Durations

To be really useful the field should also support ISO 8601 duration strings. But this requires an external library. Conversion from Python timedelta to ISO 8601 duration string and vice versa, although certainly doable, is a pain you don't easily forget.

@philtay

This comment has been minimized.

Contributor

philtay commented Feb 15, 2015

@sloria Are you ok with using integers instead of floats for this field? Have a look here:

https://docs.python.org/3.5/library/datetime.html#timedelta-objects

If no argument is a float, the conversion and normalization processes are exact (no information is lost).

I will add a microseconds boolean attribute to the field's init. If true the integer value is interpreted as microseconds during (de)serialization, otherwise as seconds. Defaults to False.

Proof of concept:

class TimeDelta(Field):
    def __init__(self, microseconds=False, error=None, **kwargs):
        self.microseconds = microseconds
        super(TimeDelta, self).__init__(error=error, **kwargs)

    def _serialize(self, value, attr, obj):
        seconds = value.days * 86400 +  value.seconds
        return seconds * 10**6 + value.microseconds if self.microseconds else seconds

    def _deserialize(self, value):
        if self.microseconds:
            return dt.timedelta(microseconds=int(value))
        else:
            return dt.timedelta(seconds=int(value))
@sloria

This comment has been minimized.

Member

sloria commented Feb 15, 2015

@philtay +1. Let's do this for 2.0-a (since this is technically a backwards-compatible change).

@philtay

This comment has been minimized.

Contributor

philtay commented Feb 15, 2015

Ok, add the 2.0-a milestone to this issue. I just need to understand why this

https://github.com/marshmallow-code/marshmallow/blob/dev/marshmallow/compat.py#L61

is necessary for Python 2.6. I think we can remove it. BTW the 2.0 release could drop Python 2.6 support. It's too legacy nowadays.

@sloria sloria added this to the 2.0-a milestone Feb 15, 2015

@sloria

This comment has been minimized.

Member

sloria commented Feb 15, 2015

The total_seconds function is needed for py26 because timedelta.total_seconds() was added in py27.

I'm not opposed to dropping py26 support, but I'm hesitant to do so unless it becomes a hindrance to development, which it hasn't (yet). We can revisit the issue if becomes more of a problem.

@philtay

This comment has been minimized.

Contributor

philtay commented Feb 15, 2015

ah, good, i'll remove it from compat. it's useless

@sloria

This comment has been minimized.

Member

sloria commented Feb 15, 2015

👍

@philtay philtay referenced this issue Feb 15, 2015

Merged

TimeDelta #148

@sloria

This comment has been minimized.

Member

sloria commented Feb 20, 2015

Closed by #148

@sloria sloria closed this Feb 20, 2015

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