-
Notifications
You must be signed in to change notification settings - Fork 176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Calculations of years and months wrong due to leap years #156
Comments
Ah dates, always more complicated than I think. I think your change in #157 is worthy but suffers from a similar problem as the issue reported in #152—these are arguably breaking changes. And because this library is in maintenance mode, I'm hesitant. However, they are also arguably bug fixes, in which case they would be in scope. What do you think? |
I agree that they are breaking changes if someone is testing the specific output against a specific input, this is evidenced by the fact that I had to make a bunch of little tweaks to the unit tests. That said, anyone expecting that level of specificity probably has already noticed this was "wrong" and wrote off the package or the use where they were not rounding. MOST people would have likely went to rounding the years (most likely) or if they really cared, are using some library where the exact date is being displayed relatively to now. I think this library is more likely used for durations (not points-in-time) and most durations in the many-month range are not likely to want exactness... and those that do want exactness in the output would like it to be accurate as well. TL;DR I think it's a bug worth fixing and unlikely to break people that would be surprised by the definition of 365.2425 :) Thanks for the introspection, I appreciate that in a library author and respect whichever way you go with this library. If you do merge it, I would expect Edit: clarify that a minor or major version bump would be appropriate |
I don't think this would warrant a minor version bump. That would be "if new, backwards compatible functionality is introduced to the public API" per the semver spec. To me, it seems like it's either a patch-level change (because it's a bugfix) or a major version bump (because it's a breaking change). I think there's an argument to be made for either. I don't feel very strongly, but I'd rather do a major version bump because:
I'd be willing to do a major version bump for something like that, even if the library is in maintenance mode. What do you think? (Sorry for the delay in responding!) |
As to semver, sure... major bump would be reasonable as well. I said minor because it isn't a patch, but it's also not an API change... but major version bump is reasonable as well. This discussion is why I didn't do any of the release-activity sort of work :) TL;DR |
Did some more thinking about this, finally. I think there are a few distinct questions at hand:
The answer to the first question is easy: if we change anything, then it's a breaking change. (I started a branch for the next major version if you want to follow along.) The other two questions are trickier. Wikipedia provides a list of time units. Milliseconds, seconds, minutes, hours, days, and weeks are constant. Months and years, however, have a range. In other words, it's easy to answer "how long is an hour, in milliseconds?" and impossible to answer "how long is a month, in milliseconds?" To inform what we should do, I looked at what other folks do.
Many others don't deal with this problem at all:
My rough takeaway from the above: if your library deals with exact durations (e.g., "1.7 minutes" instead of "about 2 minutes"), then you either (1) don't deal with months and years (2) treat them carefully. I'd like to continue to support months and years, because I think they're useful. Given that, I think HumanizeDuration.js should:
If that sounds good, I'll do this in the next major version. |
Hi! Is this being worked on? I'm not sure this would be the cause but I'm getting a weird result using a time diff from luxon that results in 31536000000ms (times are just from month/yeah - 05/2021 to 05/2022) and result this lib is giving me is 11 months, instead of 12. Sorry for maybe necroing an issue, but it is still open! Thanks. |
I'm not aware of anyone actively working on this.
I haven't had time to get to it, unfortunately.
|
The constants used for milliseconds in a year and milliseconds in a month are incorrect because they are computed from the slightly naïve understanding of leap days in the Gregorian calendar.
A leap day occurs every four years. However, because that's actually slightly too often, if the year is divisible by 100 then it is not a leap year. However, because that would be too infrequent, if the year is divisible by 400 they it is a leap year.
Thus the correct decimal representation of the average length of a year is
365.2425
days. Likewise, since the year is 12 exact months, an average length of a month is30.436875
days.The text was updated successfully, but these errors were encountered: