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 methods visibility in multiple Providers #332

Merged

Conversation

attepulkkinen
Copy link
Contributor

@attepulkkinen attepulkkinen commented Jan 15, 2024

This resolves #331

I'm mildly annoyed that some methods are named calculateHolidayName() and some are addHolidayName() and there are even holidayName() but I'm not sure if renaming these would break some backward compatibility somewhere…

I decided against adding tests because this commit doesn't include any logic changes.

Methods in several providers including Switzerland, South Korea, and Luxembourg among others have been updated from private to protected. These changes allow for better extensibility and customization of the Yasumi library. The methods affected are those calculating specific holidays in the different regions.
@attepulkkinen attepulkkinen force-pushed the update-provider-method-visibilities branch from abe226d to 41851f7 Compare January 15, 2024 12:12
@attepulkkinen attepulkkinen marked this pull request as ready for review January 15, 2024 12:14
@stelgenhof stelgenhof self-requested a review January 15, 2024 23:39
Copy link
Member

@stelgenhof stelgenhof left a comment

Choose a reason for hiding this comment

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

The changes look good, except you have some unresolved code style issues. You can fix these by running the composer format command. After that, this PR should be good to go :)

@attepulkkinen
Copy link
Contributor Author

Running that command on my computer gives no errors, but I'll write a fix manually.

Also, not sure if this is a known issue, but there is also the following deprecation warning

Detected deprecations in use:
- Rule set "@PER" is deprecated. Use "@PER-CS" instead.

@attepulkkinen
Copy link
Contributor Author

attepulkkinen commented Jan 16, 2024

Actually, this is a bit silly style fix. I don't think such type Australia\string actually exists… How do you want to continue?

--- /home/runner/work/yasumi/yasumi/src/Yasumi/Provider/Australia/AustralianCapitalTerritory.php
+++ /home/runner/work/yasumi/yasumi/src/Yasumi/Provider/Australia/AustralianCapitalTerritory.php
@@ -61,11 +61,11 @@
      *
      * @see https://en.wikipedia.org/wiki/Easter
      *
-     * @param int         $year     the year for which Easter Saturday need to be created
-     * @param string      $timezone the timezone in which Easter Saturday is celebrated
-     * @param string      $locale   the locale for which Easter Saturday need to be displayed in
-     * @param string|null $type     The type of holiday. Use the following constants: TYPE_OFFICIAL, TYPE_OBSERVANCE,
-     *                              TYPE_SEASON, TYPE_BANK or TYPE_OTHER. By default an official holiday is considered.
+     * @param int                   $year     the year for which Easter Saturday need to be created
+     * @param string                $timezone the timezone in which Easter Saturday is celebrated
+     * @param string                $locale   the locale for which Easter Saturday need to be displayed in
+     * @param Australia\string|null $type     The type of holiday. Use the following constants: TYPE_OFFICIAL, TYPE_OBSERVANCE,
+     *                                        TYPE_SEASON, TYPE_BANK or TYPE_OTHER. By default an official holiday is considered.
      *
      * @throws \Exception
      */

This seems to be related to friendsofphp/php-cs-fixer version upgrade (v3.46.0 => v3.47.0)

The long-term solution here would be to use enums when support for PHP 8.0 will be eventually dropped.

@stelgenhof
Copy link
Member

stelgenhof commented Jan 16, 2024

Thanks for noticing the deprecated ruleset. With recent PHP CS fixer updates I have seen some similar odd changes (and reverted).

Actually, this is a bit silly style fix. I don't think such type Australia\string actually exists… How do you want to continue?

Yes this indeed is a bit silly. Let me see if I can correct all of this. Although not a fan of it (and shouldn't be necessary), perhaps we can pin PHP CS fixer to a specific version.

@stelgenhof
Copy link
Member

@attepulkkinen I've made a small (hopefully temporary) change that addresses the latest PHP CS Fixer issues. You may want to rebase your branch and give it another try.

…oduces undesired changes

Signed-off-by: Sacha Telgenhof <me@sachatelgenhof.com>
@stelgenhof stelgenhof merged commit b25bca7 into azuyalabs:develop Jan 16, 2024
24 checks passed
@attepulkkinen
Copy link
Contributor Author

Thank you. Can you tell when the next version will be released?

@stelgenhof
Copy link
Member

@attepulkkinen Apologies for missing your last question. Generally I release a new version if there are a number of valuable changes since the last release. So, at the moment, I don't have any new release scheduled soon.

stelgenhof pushed a commit that referenced this pull request Oct 24, 2024
* Update methods visibility in multiple Providers

Methods in several providers including Switzerland, South Korea, and Luxembourg among others have been updated from private to protected. These changes allow for better extensibility and customization of the Yasumi library. The methods affected are those calculating specific holidays in the different regions.
stelgenhof pushed a commit that referenced this pull request Oct 24, 2024
* Update methods visibility in multiple Providers

Methods in several providers including Switzerland, South Korea, and Luxembourg among others have been updated from private to protected. These changes allow for better extensibility and customization of the Yasumi library. The methods affected are those calculating specific holidays in the different regions.
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.

Holiday calculation functions in providers are private, could those be protected instead?
2 participants