-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
add yasumi creation by ISO3166-2 code #62
Conversation
src/Yasumi/Yasumi.php
Outdated
{ | ||
$providers = self::getProviders(); | ||
|
||
$class = isset($providers[$iso3166_2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify this by combining this and the 'if ($class === false)
' into one. Then no need for the intermediate variable $class
and ternary check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I am not sure about.
I can see that the assign of $providers
is kind of unnecessary since the getProviders
uses a static internal variable preventing the repetitive directory scanning.
But I am not convinced that something like:
if ( false === isset( self::getProviders()[$iso3166_2] ) ) {
throw new InvalidArgumentException(sprintf('Unable to find holiday provider by ISO3166-2 "%s".', $iso3166_2));
}
return self::create(
self::getProviders()[$iso3166_2],
$year,
$locale
);
benefits the readability of the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see what you mean. What about this way:
$providers = self::getProviders();
if ( false === isset( $providers[$iso3166_2] ) ) {
throw new InvalidArgumentException(sprintf('Unable to find holiday provider by ISO3166-2 "%s".', $iso3166_2));
}
return self::create(
$providers[$iso3166_2],
$year,
$locale
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deal :)
src/Yasumi/Yasumi.php
Outdated
* @throws RuntimeException If no such holiday provider is found | ||
* @throws InvalidArgumentException if the year parameter is not between 1000 and 9999 | ||
* @throws UnknownLocaleException if the locale parameter is invalid | ||
* @throws InvalidArgumentException if the holiday provider for the given country does not exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be corrected as the parameter is not 'country' but 'ISO3166-2'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
@@ -186,6 +186,44 @@ public static function create($class, $year = null, $locale = self::DEFAULT_LOCA | |||
} | |||
|
|||
/** | |||
* Create a new holiday provider instance. | |||
* | |||
* A new holiday provider instance can be created using this function. You can use one of the providers included |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description needs to be edited since this function is using the 'ISO3166-2' code as the input parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looking good
See issue
I needed the option to create a yasumi instance by ISO3166-2 Code. As far as I can see, the
const ID
in the providers corresponces to this code. Therefore I created this simple wrapper.I wasn't able to create test etc. for this wrapper. I think this should be added before mergen