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

update offsets.pyx to fix #60647 #61181

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kangqiwang
Copy link

@kangqiwang kangqiwang requested a review from WillAyd as a code owner March 26, 2025 11:51
@kangqiwang
Copy link
Author

Sorry for not adding a test case for this. I'll set it up once I get home and can work on my PC—my current laptop is too slow to handle the full testing environment.

@kangqiwang
Copy link
Author

Also, I noticed the unit tests failed. Again, I will fix them once I get home.

@a-holm
Copy link

a-holm commented Mar 31, 2025

Thanks for working on this fix for #60647!

This change successfully removes the code path containing the specific line (holidays = holidays + calendar.holidays().tolist()) identified in the issue and the comments.

However, there are a few points that probably deserve some attention:

Removed Functionality: This fix works by removing the if-else block. Was the functionality within that block (handling a non-numpy calendar object passed with holidays) intended or ever valid? Removing it is a potentially breaking change that should ideally be documented.

Exception Type: Raising ApplyTypeError here is a bit unusual, as it's typically for arithmetic operations returning NotImplemented. Would a ValueError or TypeError be more appropriate to signal invalid arguments during the setup phase?

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.

BUG: CustomBusinessDay not respecting calendar
2 participants