Skip to content
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

Handle leap day birthdays by moving them one day earlier #64

Merged
merged 1 commit into from
May 17, 2020

Conversation

awendland
Copy link
Contributor

This fix is unideal for the long term (since every four years these birthdays will be 1 day off) but I thought that it might be more useful in the short term than having the program exit.

@johnameen
Copy link

The reason leap years break his code is actually because it inserts the leap year birthday out of order-- in between all the 02/28 birthdays. If you move the 02/29 birthday to after all the 02/29 birthdays in the list, you won't have to incorrectly define someone's birthday as 02/28.

@awendland
Copy link
Contributor Author

awendland commented May 10, 2020

@johnameen I'm going to need some more assistance understanding the error case that you're talking about.

The error I'm addressing here (which I should have included in my hastily written PR description) is an exception being thrown by the datetime module when it's being supplied an invalid date (since February 29th doesn't occur in every year) through e.begin.

Here's an example of replicating the error in a Python REPL:

>>> from datetime import date
>>> date(2020, 2, 29)
datetime.date(2020, 2, 29)
>>> date(2021, 2, 29)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: day is out of range for month

This program encounters the error case because it creates a calendar event in the next year for birthdays that have already occurred in the given year (which happens to be a leap year, otherwise this error wouldn't occur).

This fix is unideal for the long term (since every four years these
birthdays will be 1 day off) but in the short term it's more useful than
having the program exit.
@mobeigi
Copy link
Owner

mobeigi commented May 17, 2020

@awendland Thanks for this! I slightly modified your commit hope you don't mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants