isless for CompoundPeriod and Period combinations #37392
Conversation
, should technically be faster for allowe comparisions
checked properly and actually work
stdlib/Dates/test/periods.jl
Outdated
| @test (2y < 25m+1y) == true | ||
| @test (25m+1y < 2y) == false | ||
| #tests for error throwing for not allowed comparisons (FixedPeriod and OtherPeriod combinations) | ||
| @test_throws ErrorException y + h < h + m |
There was a problem hiding this comment.
But even in this case, there's an obvious answer here: 1 year + 1 hour is not less than 1 hour + 1 month. That's why I think we should just convert the whole CompoundPeriod with Dates.tons and compare the final answers. Not worrying about fixed vs. non-fixed. Are there actually awkward cases if we do that?
There was a problem hiding this comment.
Yeah i agree it is true, i just didnt want to create a comparision that is different to how they seem to be treating other comparisions between the types.
Id rather it allowed the comparision as id hate getting a random error when it is obvious that Year(1) + Hour(1) < Month(1) but it would be weird if it was allowed but then if you tried to test if Year(1) < Hour(1) it gave an error.
I feel making it that the error being thrown is the normal behaviour and just put somewhere that to get around the error do
tons(Year(1) + Hour(1)) < tons(Month(1)) or i could change the other less than comparision.
I just saw that there was a decently large discussion but how to handle this stuff and that is what they agreed on doing
There was a problem hiding this comment.
Sorry to bring up this older pull, but which do you think is the better idea? That error throwing is just a bit of an issue and its hard to tell whether this should follow throwing errors or simply going with what is most likely and use "tons()"
There was a problem hiding this comment.
I've just updated it to use tons and the tests
Adds the isless() operator for CompoundPeriods aswell as the combinations of CompoundPeriod and Period. CompoundPeriods that contain both FixedPeriods and OtherPeriods instead throw an error as they are not directly comparable, this should follow the same idea as it appears the rest of the periods file is using.
Tests were also created for the changes
Feature was asked for in #32389