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

Added Colombia Holidays #55

Merged
merged 10 commits into from
Jan 24, 2024
Merged

Added Colombia Holidays #55

merged 10 commits into from
Jan 24, 2024

Conversation

alvleont
Copy link
Contributor

Added the Colombia's holidays. There's a situation with some holidays due to the "Emiliany Law" they are moved to the following monday if it is not monday, so a function to deal with that was added.

Added the Colombia's holidays. There's a situation with some holidays due to the "Emiliany Law" they are moved to the following monday.
@alvleont alvleont closed this Jan 18, 2024
@alvleont alvleont reopened this Jan 18, 2024
Copy link
Member

@Nielsvanpach Nielsvanpach left a comment

Choose a reason for hiding this comment

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

Thanks! Could you rebase your branch on main, so I can run the tests here?

src/Countries/Colombia.php Outdated Show resolved Hide resolved
Added the Colombia's holidays. There's a situation with some holidays due to the "Emiliany Law" they are moved to the following monday.
Deleted the line as suggested by @Nielsvanpach
@alvleont
Copy link
Contributor Author

@Nielsvanpach Ok. I'm pretty sure I messed up something.

@Nielsvanpach
Copy link
Member

No worries. Make sure your fork is in sync with this package:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

You could always try a new branch from main and make a new PR

@alvleont
Copy link
Contributor Author

Done.

Copy link

@luisprmat luisprmat left a comment

Choose a reason for hiding this comment

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

This implementation looks correct, I also tested it.

@Nielsvanpach
Copy link
Member

Could you verify if the dates are correct in the generated snapshot? At the moment the test is failing

@luisprmat
Copy link

luisprmat commented Jan 19, 2024

Could you verify if the dates are correct in the generated snapshot? At the moment the test is failing

@Nielsvanpach The dates are correct and they make match with snapshot, the problem seems to be with the data types for emilianiHoliday() params.
If we change to

private function emilianiHoliday(CarbonImmutable|bool $date = false)

??

@Nielsvanpach
Copy link
Member

I'm not aware of the situation in Colombia. I'll leave this PR open for other Colombians to discuss

@luisprmat luisprmat mentioned this pull request Jan 20, 2024
1 task
@alvleont
Copy link
Contributor Author

Ok, I did some extra reviews and it seems there was some problems with "Dia de San José" date, and also because all Emiliani dates timezone were not set. So @Nielsvanpach @luisprmat , I guess everything should work fine now.

Copy link

@luisprmat luisprmat left a comment

Choose a reason for hiding this comment

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

Hi @alvleont, your change of 'March 19' to 'March 20' for "Día de san José" may work but it is not the right thing to do. Testing with your code I also found an error in 2026 with the day of the "Reyes magos"; in the snapshot it is obtained on "2026-01-05" but it should be "2026-01-12".

Reviewing I found the following problems for which I propose these solutions:

Problem 1 - The delays of one day in some cases are due to the creation of the dates on the GitHub actions test server, since if it is very close at midnight it is created for the next day and therefore it does not match the snapshot. Solution: After creating the date with CarbonImmutable we chain the method ->startOfDay() to reset the time to 00:00:00 so I no longer see the need to use timezone.

Problem 2 - The phpstan analysis fails because the data type of the method CarbonImmutable::createFromFormat returns an union type CarbonImmutable|false but the emilianiHoliday method expects a parameter with only CarbonImmutable type. Solution: Use the method CarbonImmutable::createFromDate($year, $month, $day, $timezone?), this method returns only CarbonImmutable and it does not generate problems with phpstan.

CONCLUSION
This is my proposal for src/Countries/Colombia.php

<?php

namespace Spatie\Holidays\Countries;

use Carbon\CarbonImmutable;

class Colombia extends Country
{
    public function countryCode(): string
    {
        return 'co';
    }

    protected function allHolidays(int $year): array
    {
        return array_merge([
            'Año Nuevo' => '01-01',
            'Día del Trabajo' => '05-01',
            'Día de la independencia' => '07-20',
            'Batalla de Boyacá' => '08-07',
            'Inmaculada Concepción' => '12-08',
            'Navidad' => '12-25',
        ], $this->variableHolidays($year));
    }

    /** @return array<string, CarbonImmutable> */
    protected function variableHolidays(int $year): array
    {
        $easter = $this->easter($year);

        return [
            'Reyes Magos' => $this->emilianiHoliday(CarbonImmutable::createFromDate($year, 1, 6)->startOfDay()),
            'Día de San José' => $this->emilianiHoliday(CarbonImmutable::createFromDate($year, 3, 19)->startOfDay()),
            'San Pedro y San Pablo' => $this->emilianiHoliday(CarbonImmutable::createFromDate($year, 6, 29)->startOfDay()),
            'Asunción de la Virgen' => $this->emilianiHoliday(CarbonImmutable::createFromDate($year, 8, 15)->startOfDay()),
            'Día de la raza' => $this->emilianiHoliday(CarbonImmutable::createFromDate($year, 10, 12)->startOfDay()),
            'Todos los santos' => $this->emilianiHoliday(CarbonImmutable::createFromDate($year, 11, 1)->startOfDay()),
            'Independencia de Cartagena' => $this->emilianiHoliday(CarbonImmutable::createFromDate($year, 11, 11)->startOfDay()),
            'Jueves Santo' => $easter->subDays(3),
            'Viernes Santo' => $easter->subDays(2),
            'Ascención de Jesús' => $easter->addDays(43),
            'Corpus Christi' => $easter->addDays(64),
            'Sagrado corazón de Jesús' => $easter->addDays(71),

        ];
    }

    /** @return CarbonImmutable */
    private function emilianiHoliday(CarbonImmutable $date): CarbonImmutable
    {
        if ($date->is('Monday')) {
            return $date;
        } else {
            return $date->next('Monday');
        }
    }
}

I also suggest changing emilianiHoliday to something more descriptive like movableHoliday or something, but it's not as relevant.

alvleont and others added 2 commits January 23, 2024 10:25
- Suggestions from @luisprmat were incorporated.
- All the work of emiliani holiday generation was included into the emilianiHoliday method to achieve code readability
- The order of the holidays was updated to ensure that the next emiliani holiday that appears can be included at the end of the list.
- The test was updated because it's "colombian holidays" not "colombia holidays"
@alvleont alvleont requested a review from luisprmat January 23, 2024 15:27
Copy link

@luisprmat luisprmat left a comment

Choose a reason for hiding this comment

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

Reviewing, I tested from 2016 to 2026 (in my PC, time for Colombia) and i didn't find errors. The code looks good and I think we should merge it, if an error arises later we will report it and we will resolve it.

@luisprmat
Copy link

@Nielsvanpach please review!

@luisprmat luisprmat mentioned this pull request Jan 23, 2024
1 task
Copy link

@juancho48 juancho48 left a comment

Choose a reason for hiding this comment

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

This is perfect! Will withdraw mine

Copy link
Member

@Nielsvanpach Nielsvanpach left a comment

Choose a reason for hiding this comment

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

Thanks!

@Nielsvanpach Nielsvanpach merged commit 82afddc into spatie:main Jan 24, 2024
8 checks passed
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.

4 participants