fitnr / convertdate

Utils for converting between date formats and calculating holidays
MIT License
47 stars 26 forks source link

Swap Adar I and Adar II in Hebrew #4

Closed gregarkhipov closed 8 years ago

gregarkhipov commented 8 years ago

Hello, a nice work you've done here!

I wanted to use this tool for our python-powered site and noticed that Adar months in leap year are not where they're supposed to be. The thing is that in Hebrew calendar Adar I is additional. I.e., a person born in Adar in a common Hebrew year would celebrate his birthday in Adar Bet (the second) in a leap year. Same thing with the Holidays.

So this way, hebrew.from_gregorian() should return consequently ... — 10 — 11 — 13 — 12— 1 — ... for a leap year. It might seem irrational, but this is the way Hebrew calendar works.

Since your package is on PyPI, I realize that this change will result in existing apps using this library not working properly when they update the library. But I assume nobody yet used it for Hebrew, because they would definitely notice the issue.

Although, I could make a pull request, but later, when I'm less busy.

fitnr commented 8 years ago

Thanks for bringing this up. I think the converter is currently working in the expected fashion. At least, it agrees with every other converter and joint listing I can find (1, 2, 3)

In embolismic years, the 12th month is Adar I, and the 13th is Adar II, so it seems appropriate for hebrew.from_gregorian() to return 12 and 13 respectively. If you would like to track events that usually occur in the 12th month, but sometimes in the 13th (e.g. Purim), I suggest using hebrew.leap() to check if it is a leap year.

If I've misunderstood what you're asking, it would be helpful to see a test case that the library would currently fail.

gregarkhipov commented 8 years ago

I already adapted it for our project via using hebrew.leap().

And of course, for computing purposes Adar I is logically the 12th month, but I judge it from halachic point of view. And also my code could have been simpler if the months was swapped. Because to calculate birthdays and Holidays I have to either manually swap 12 and 13 for leap year, or redirect 12th month's events to 13th month anyways. Although, I would still need to use leap() method to set name for 12th month (Adar or Adar II), but it feels more natural (It remains the same month).

Maybe if chabad.org call a leap year's 12th month Adar I for their converter, then it's OK. But I couldn't find it out from your reference. What exactly did you mean?

fitnr commented 8 years ago

I understand that it's a pain to deal with leap months, but I think the current situation is the most general, easiest-to-understand solution.

Chabad.org doesn't explicitly list Adar I as the 12th month, but it's the 12th month that occurs in their dual-system calendar for AM 5776.

Glad you're finding the library (at least somewhat) useful!

gregarkhipov commented 8 years ago

I didn't state that consequently Adar I is 13'th month, I know it's 12th, if you count from Nissan. I just said it's more profound to make it 13th, even from OOP point of view (since Adar and Adar II have the same events). BTW, additional Adar comes explicitly before the common Adar, unlike in other calendars, so there's no comparison. But I'm cool with the current state, if you don't wanna touch it. It still serves for good things.

Happy Chanukah!