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

Refactor days offset calculation: replace relativedelta with timedelta #900

Merged
merged 7 commits into from
Feb 1, 2023

Conversation

arkid15r
Copy link
Collaborator

@arkid15r arkid15r commented Jan 21, 2023

Resolves #891

This PR contains business logic only changes. See #901 for tests changes.

My simple benchmark results for the following code:

def create_country_holidays():
    for country in list_supported_countries():
        country_holidays(country, years=range(2020, 2031))
  • the relativedelta:
timeit.timeit(create_country_holidays, number=1000)
339.802383083
  • the timedelta:
timeit.timeit(create_country_holidays, number=1000)
313.579699042

@coveralls
Copy link

coveralls commented Jan 21, 2023

Coverage Status

Coverage: 100.0%. Remained the same when pulling f49aa89 on arkid15r:timedelta-migration into c8b2d0c on dr-prodigy:beta.

@@ -9,10 +9,9 @@
# Website: https://github.com/dr-prodigy/python-holidays
# License: MIT (see LICENSE file)

from datetime import date
from datetime import date, timedelta
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe import timedelta as td? Then lines length will be same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it was my original intent to do it that way. I'm not sure why I didn't stick to the plan. Thank for pointing out, @KJhellico!

It's great to have you back! It felt somewhat empty without your reviews here. I really appreciate them!

@arkid15r arkid15r added the ready for beta Ready to merge on beta branch label Jan 31, 2023
@arkid15r arkid15r changed the title Replace relativedelta with timedelta for days offset calculation Refactor days offset calculation: replace relativedelta with timedelta Feb 1, 2023
@arkid15r arkid15r merged commit acd0a06 into vacanza:beta Feb 1, 2023
@arkid15r arkid15r deleted the timedelta-migration branch February 1, 2023 19:15
@arkid15r arkid15r removed the ready for beta Ready to merge on beta branch label Feb 1, 2023
This was referenced Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use timedelta for days offset calculation
3 participants