sdispater / pendulum

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

Incorrect duration parsing logic with decimals #534

Open jacobg opened 3 years ago

jacobg commented 3 years ago
>>> pendulum.parse('PT16M')
Duration(minutes=16) # GOOD!

>>> pendulum.parse('PT16.4M')
Duration(minutes=16, seconds=24)  # GOOD!

>>> pendulum.parse('PT16.48M')
Duration(minutes=20, seconds=48). # BAD! Should be: Duration(minutes=16, seconds=28, milliseconds=800)

The first two cases above have the correct output. The third one is not correct.

robjhornby commented 3 years ago

Just looking out of interest. This is handled by _parse_iso8601_duration.

The number of digits after the decimal marker is unlimited in the regex to capture minutes:

https://github.com/sdispater/pendulum/blob/411d0aa41a5f39c5f4a2f43a3c369c2dd24787db/pendulum/parsing/iso8601.py#L74

but is assumed to be a single digit when converting it to seconds (dividing by 10 rather than taking the number of digits into account) so it's putting 4.8 minutes into the seconds part instead of 0.48 minutes.

https://github.com/sdispater/pendulum/blob/411d0aa41a5f39c5f4a2f43a3c369c2dd24787db/pendulum/parsing/iso8601.py#L384

Also, these values are then passed to the Duration class, which has its own normalization of days/hours/minutes/seconds as well:

https://github.com/sdispater/pendulum/blob/411d0aa41a5f39c5f4a2f43a3c369c2dd24787db/pendulum/duration.py#L79-L90

e.g. this works as expected:

>>> pendulum.Duration(minutes=16.48)
Duration(minutes=16, seconds=28, microseconds=800000)

So maybe _parse_iso8601_duration can be simplified