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

Clean up timedelta/relativedelta usage #892

Merged
merged 4 commits into from
Jan 17, 2023

Conversation

arkid15r
Copy link
Collaborator

  • Standardize relativedelta days offset usage
  • Replace timedelta with relativedelta

Replace `timedelta` with `relativedelta`.
@coveralls
Copy link

coveralls commented Jan 12, 2023

Coverage Status

Coverage: 100.0%. Remained the same when pulling 56ac25a on arkid15r:relative-delta-clean-up into 84af05e on dr-prodigy:beta.

@dr-prodigy
Copy link
Collaborator

Hi @arkid15r , I like the idea behind this PR a lot! although I have an important note to add: given this refactoring is very large, I would prefer to have it split in 2, ie:
1/ business logic code refactoring, no changes on tests
2/ no changes on BL, tests refactoring
so to guarantee no regressions without requiring extensive visual reviews.
Do you think it's still possible?
Thx a lot!

@arkid15r
Copy link
Collaborator Author

That's a great point, @dr-prodigy!

I implemented and tested these changes in the 2 steps you've just described. However, the PR doesn't reflect that.
It makes perfect sense to split it into 2 as you suggested. I've extracted tests into https://github.com/dr-prodigy/python-holidays/pull/894

Thanks for the review!

@dr-prodigy dr-prodigy added the ready for beta Ready to merge on beta branch label Jan 17, 2023
@arkid15r arkid15r merged commit d921655 into vacanza:beta Jan 17, 2023
@arkid15r arkid15r deleted the relative-delta-clean-up branch January 17, 2023 15:55
@arkid15r arkid15r removed the ready for beta Ready to merge on beta branch label Jan 17, 2023
@arkid15r arkid15r mentioned this pull request Jan 30, 2023
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