Do not use time.Date to convert BS dates
mesaugat opened this issue · 3 comments
I was reading through the code and saw this piece of code.
Line 37 in 065a8fb
The question that popped in my head right away is how can we possibly get the right Bikram Sambat dates if we use the inbuilt time.Date
function which possibly follows the Gregorian calendar.
The Gregorian calendar never exceeds 31 days. However, the Bikram Sambat calendar does.
I quickly checked the Nepali calendar and found June 15, 2019
to be जेठ 32, 2076
. I tried converting this date and here are the results.
Wrong Results: ❎
nepcal conv adtobs 06-15-2019
असार 3, 2076 शनिबार
nepcal conv adtobs 06-13-2019
असार 1, 2076 बिहिबार
Right Results: ✅
nepcal conv adtobs 06-18-2019
असार 3, 2076 मंगलबार
nepcal conv adtobs 06-17-2019
असार 2, 2076 सोमबार
Let's take June 15, 2019
as an example. Supposedly, the conversion is right and we get year, month, days to be 2076, 02, 32
which translates to जेठ 32, 2076
.
Now when we pass this result to toTime
it will try to convert these dates to Gregorian calendar. If you take a look at the 2076 Gregorian calendar, the month of February (02) has only 29 days. Amazingly, what time.Date
tries to do here is append the excess of 3 days and add it to the next month. This results in the output of 2076, 03, 03
which is असार 3, 2076
.
There might be plethora of dates that don't convert well. 😛
Thanks for finding the issue, and for the detailed breakdown as well.
If I understand correctly, the problem is as follows:
If a B.S. date (say जेठ 32
) is represented as 02/32
(which is the case right now), toTime
will (obviously) interpret it as Feb 32
which is an overflow by 32 - 29 = 3
days. So it's reinterpreted by the time
package to be March 3
which is interpreted by nepcal
to be असार 3
.
This is problematic for sure. I guess the fix is to create a BSDate
struct which holds these values and return that. This struct could also hold the eventual toAD()
method. Using this struct would also imply changes to the main
package which works with time.Date
. I think this is the scope of what needs to be done - shouldn't be too difficult to make it work with the new types.
Thanks again!
For completeness, the following test cases should pass:
AD Date | Current BS Output | Expected Output |
---|---|---|
June 15, 2019 | असार 3, 2076 | जेठ 32, 2076 |
June 13, 2019 | असार 1, 2076 | जेठ 30, 2076 |
Presumably, it fails because the following happens inside the time
package.
yy, mm, dd = func() { .. }()
if adMonth(mm).hasLessDaysThan(dd) {
mm = mm + 1
dd = dd - numberOfDaysInAdMonth(mm) // overflow
}
Fixed and released in v0.5.0!