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

Support for Bulgarian holidays #132

Closed
wants to merge 13 commits into from
Closed

Conversation

dyanakiev
Copy link

Contributing a new country ?

  • Have you checked to ensure there aren't other open Pull Requests for the same country?
  1. Create a new class in the Countries directory. It should extend the Country class.
  2. Add a test for the new country in the tests directory.
  3. Run the tests so a snapshot gets created.
  4. Verify the result in the newly created snapshot is correct.

* @return CarbonImmutable
* @throws \RuntimeException if the calculated date is invalid
*/
protected function calculateEasterDate($year): CarbonImmutable
Copy link
Member

Choose a reason for hiding this comment

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

Is this an orthodox calculation? We already have one in Country.php. Does that one work too?

Copy link
Author

Choose a reason for hiding this comment

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

I totally didn't see that. Yes it works for Bulgaria too.

Copy link
Member

Choose a reason for hiding this comment

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

I think you used the wrong one.

Copy link
Author

Choose a reason for hiding this comment

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

Yep you're right.. Its correct for 2024 5 May 2024, but for 2025 it should be 20 April 2025. I'm not sure which calculation to use..

Copy link
Author

Choose a reason for hiding this comment

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

Ok using the easter_date function directly actually resolves the issue.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, nevermind.. 🤣 It's now wrong for 2024, im confused..

Copy link
Member

Choose a reason for hiding this comment

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

Country.php has a easter() and orthodoxEaster() function. I suggest to use one of those.

If the orthodox calculation gives a false result, feel free to update it, in order to make it work. Do verify if the results for NorthMacedonia is still correct.

Copy link
Author

Choose a reason for hiding this comment

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

Neither functions give proper result, i will investigate more.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that this function in Country.php is wrong calculating the OrthodoxEastern. Please check Greece #114 to fix the problem. @Nielsvanpach needs to update orthodoxEaster()

Country.php has a easter() and orthodoxEaster() function.

Copy link
Member

Choose a reason for hiding this comment

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

It's still the wrong function you're calling. You should use orthodoxEaster()

src/Countries/Bulgaria.php Show resolved Hide resolved
src/Countries/Bulgaria.php Outdated Show resolved Hide resolved
src/Countries/Bulgaria.php Outdated Show resolved Hide resolved
@@ -0,0 +1,46 @@
[
{
"name": "\u041d\u043e\u0432\u0430 \u0433\u043e\u0434\u0438\u043d\u0430",
Copy link
Member

Choose a reason for hiding this comment

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

If needed, you ca also add a translation file to the lang directory

dyanakiev and others added 2 commits February 13, 2024 15:00
Co-authored-by: Niels Vanpachtenbeke <[email protected]>
Co-authored-by: Niels Vanpachtenbeke <[email protected]>
@Nielsvanpach
Copy link
Member

Could you have a look at the failing test?

@spatie-bot
Copy link

Dear contributor,

because this pull request seems to be inactive for quite some time now, I've automatically closed it. If you feel this pull request deserves some attention from my human colleagues feel free to reopen it.

@spatie-bot spatie-bot closed this Jun 14, 2024
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