pendulum icon indicating copy to clipboard operation
pendulum copied to clipboard

Incorrect duration parsing logic with decimals

Open jacobg opened this issue 4 years ago • 1 comments

>>> 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.

jacobg avatar Feb 14 '21 19:02 jacobg

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

robjhornby avatar Aug 29 '21 22:08 robjhornby