search

azuyalabs / yasumi

The easy PHP Library for calculating holidays
https://www.yasumi.dev
Other
1.04k stars 152 forks source link

TypeHinting for public functions #76

Closed huehnerhose closed 6 years ago

huehnerhose commented 6 years ago

Like mentioned in #75 I would prefer type hinting as solution. Since TypeHinting doesn't allow default parameter values other than null, we have to default them manually.

huehnerhose commented 6 years ago

If I am not mistaken, reverting this to 12645bc should pass on php5.6 as well.

huehnerhose commented 6 years ago

I reverted this and switched back to the ternary version. But Travis CI doesn't seem to like this very much. I ran the test locally with success.

Honestly I am puzzled by the errors raised, especially on the 5.6 environment claiming to expect a Yasumi\string.

stelgenhof commented 6 years ago

Your type hint uses 'string' in the current namespace 'Yasumi', which doesn't exist of course. Also 'string' as a type was only introduced as of version 7 :( See: http://php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration

I guess it's time to make use of PHP 7 stuff in Yasumi :)

huehnerhose commented 6 years ago

Urg... didn't realized that type hinting first came for not scalar types. Sorry.

I don't know how important php5 support ist for you / your users. If you consider it valuable we can make the check "by hand" inside the functions and manually throwing exceptions.

huehnerhose commented 6 years ago

I wanted to draft a php5 version https://github.com/InSitu-Software/yasumi/tree/feature/typehinted5 but now all tests fail with

Yasumi\tests\Ukraine\SecondInternationalWorkersDayTest::testSecondInternationalWorkersDay with data set #9 ('1989', DateTime Object (...))
InvalidArgumentException: integer expected, string given

😅

stelgenhof commented 6 years ago

I'll create a branch for the PHP7 development. It's something I've been planning but haven't gotten around to. Maybe better to make all your changes there. In the near future I'd like to end PHP5 support and continue to use PHP7.

stelgenhof commented 6 years ago

PHP7 branch created :). Please go ahead and use that for any PHP7 related enhancements.

stelgenhof commented 6 years ago

I've updated the travis configuration so it should work now.