Add Spanish Holidays #34
Conversation
|
Hello, first thanks for the PR, but there are some things that need to be fixed:
Then there is an issue and it is that national holidays do not always fall on the same days, sometimes the 1st of Jan if it falls on a Sunday is moved to the 2nd day or something like that. How do you think this issue could be handled, or should there really be a PR each time the holidays for the following year are approved? Thank you! |
|
I think it is a problem that will occur in multiple locations, while the scope of the library only reaches countries and not country regions. I have only included national holidays that are common to all autonomous communities. It would be necessary to be able to go down a level to filter by autonomous community or obtain all of them, indicating which community it belongs to (similar to the behavior of Google Calendar). Regarding the free placement of holidays, perhaps it would be necessary to be able to specify it by year, and yes, update periodically after approval. What do you think? Thanks! |
|
Could you rebase your branch on our main branch? That way I can run the tests |
|
Done. |
|
It seems like Spain has some additional holidays, based on the regions? We added some docs on how we think this could be implemented. Could you extend this PR with support? Thanks! https://github.com/spatie/holidays?tab=readme-ov-file#contributing-a-new-country |
|
Yes, I just saw it. I'll add them! |
src/Countries/Spain.php
Outdated
| return $this->regionHolidays2024(); | ||
| } | ||
|
|
||
| return []; |
There was a problem hiding this comment.
I'm not familiar with the holidays in Spain, but isn't it possible to calculate the days instead of hardcoding them? This way we can have correct data for more years.
|
National, fixed and variable holidays are always the same. Each autonomous community is free to assign additional holidays that are agreed upon before the beginning of each year. These holidays vary each year, for example, holidays are moved to Monday if they fall on a Sunday, or they are changed to another holiday if there is a long period of time without one. The problem for these cases is that there are no fixed rules that allow it to be calculated either in the future or in the past, in addition to the fact that each community has different preferences. I think the only thing that can be done is to add some history from recent years, and since we cannot obtain future dates prospectively, add new ones as these are approved. In my proposal I have tried to group and unify duplicate dates to add some clarity, but to consider regional cases I can't think of many alternatives. |
|
If it's random every year, adding a bit of historical data seems fine to me. |
|
|
||
| /** @return array<string, string> */ | ||
| protected function regionHolidays(int $year): array | ||
| { |
There was a problem hiding this comment.
$holidays = Holidays::for(Spain::make(year: 2020, region: 'es-an'))->get(); In this case I think we should throw an exception, as we won't be returning the region based holidays.
There was a problem hiding this comment.
I see, and implement some direct way to also return national holidays, which are still valid, when there is no regional information for a specific year?
There was a problem hiding this comment.
I prefer an exception, as the user clearly expects region based holidays, which we cannot provide.
This can be a dedicated RuntimeException class, so it's easier to catch
There was a problem hiding this comment.
You can use InvalidYear::rang()
No description provided.