Fix parsing of fractional durations #13832
Conversation
urmastalimaa
force-pushed
the
fix_fractional_duration_parsing
branch
2 times, most recently
from
20f9478 to
dbdd9be
Compare
|
cc @tfiedlerdejanze you happy with this change? |
|
I'll clarify that I didn't change the possibility and interpretation of double negatives. |
|
FWIW i'd also find it counter-intuitive for
Totally missed you added this with the first test, sorry. I am fine with the fix!
this is correct, it was intended for us to support fractional units for the smallest time unit only for now. |
urmastalimaa
force-pushed
the
fix_fractional_duration_parsing
branch
2 times, most recently
from
51b9123 to
762c992
Compare
Existing behaviour is to accept fractions only in the second components
Extend tests for existing fractional durations parsing behaviour. Parsing "PT0,6S" should result in a positive microsecond, which will be fixed in the next commit. The parser currently accepts both a negative prefix "-PT" and a negative value in the components "-0,6S". Combining both into "-PT-0,6S" is also accepted by the parser, resulting in an overall positive duration. Although the practical value of double negative durations is dubious, the accepting nature of the parser is tested. The JavaScript libraries that I tested disagree whether "-PT-0,6S" represents a positive or negative duration, "moment" and "tinyduration" reported positive, "luxon" (the successor to moment) reported negative. If the parser accepts double negatives at all, resulting in anything but a positive duration would be unintuitive.
The parsing of fractional durations checked for non-negativity by testing second > 0, which reports false for not only negative integers but also for 0. Note that changing `if second > 0` to `if second >= 0` would fix behaviour for "PT0,6S", but would break "PT-0,6S".
urmastalimaa
force-pushed
the
fix_fractional_duration_parsing
branch
from
762c992 to
5678a33
Compare
|
Thanks for the context. Me getting on tangents without stressing what is being fixed caused a bit of confusion, my apologies.
|
|
💚 💙 💜 💛 ❤️ |
The parsing of fractional durations checked for non-negativity by testing second > 0, which reports false for not only negative integers but also for 0. Note that changing `if second > 0` to `if second >= 0` would fix behaviour for "PT0,6S", but would break "PT-0,6S".
The parsing of fractional durations checked for non-negativity by testing
second > 0, which reportsfalsefor not only negative integers but also for 0.The JavaScript libraries that I tested disagree whether "-PT-0,6S" represents a positive or negative duration, "moment" and "tinyduration" reported positive, "luxon" (the successor to moment) reported negative. My reading of the specification gives me no reason to doubt that a double negative should result in anything but a positive total result.
It is worth mentioning that as the scale units are represented as integers, there is no clean way to implement fractional parsing of non-second units. Currently, fractional non-second durations simply fail to parse. Wikipedia gives "0.5Y" as an example.