sdispater / pendulum

Python datetimes made easy
https://pendulum.eustace.io
MIT License
6.25k stars 387 forks source link

Error dividing duration by float #525

Closed sralloza closed 2 years ago

sralloza commented 3 years ago

Issue

In pendulum:

import pendulum

d = pendulum.duration(hours=4, minutes=5)
d / 1.1

It gives the following error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Python\lib\site-packages\pendulum\duration.py", line 396, in __truediv__
    months=_divide_and_round(self._months, other),
  File "C:\Python\lib\site-packages\pendulum\duration.py", line 66, in __new__
    raise ValueError("Float year and months are not supported")
ValueError: Float year and months are not supported

Using datetime.timedelta:

from datetime import timedelta

t = timedelta(seconds=4*3600+5*60)
t / 1.1
# datetime.timedelta(seconds=13363, microseconds=636364)

It doesn't raise any error. Both objects represent the same amount of time:

from datetime import timedelta

import pendulum

d = pendulum.duration(hours=4, minutes=5)
t = timedelta(seconds=4*3600+5*60)
assert d.as_timedelta() == t
sralloza commented 2 years ago

I've been digging in the code. This error is raised because pendulum.duration._divide_and_round returns 0.0 as the month instead of 0. Inside this function, divmod(0, 1.1) returns (0.0, 0.0) instead of (0, 0), so the variable q is always a float and then it raises the error, because the month attribute of the duration must not be a float.

NickFabry commented 2 years ago

Hi @sralloza - first, thanks for taking the time track down what was causing the error, and also for pushing a fix.

I'd like to suggest that you make your push simpler, though. Currently, you have:

    if isinstance(q, float) and q == int(q):
         q = int(q)

However, q will always be either an int, or a float with a fractional part of 0. (See https://docs.python.org/3/library/functions.html?highlight=divmod#divmod for a reference on this.) Therefore, q == int(q) will always be true. This being the case, the other part of the test, seeing if q is a float before applying int(), just makes the code slower.

I suggest modifying your push code to:

    # q can be a float if a or b is, but we always want it an int
     q = int(q)

Thanks!

And my apologies if this is a non-usual request - I'm a new maintainer, so I might be asking or explaining things in a more tedious fashion than normal. Please be patient.

sralloza commented 2 years ago

No problem at all! Next time I would suggest to comment this in the PR. You're right, I'll change it.