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

add yasumi creation by ISO3166-2 code #62

Merged
merged 5 commits into from
Jun 9, 2017
Merged

add yasumi creation by ISO3166-2 code #62

merged 5 commits into from
Jun 9, 2017

Conversation

huehnerhose
Copy link
Contributor

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

@stelgenhof stelgenhof added this to the v1.7.0 milestone Jun 6, 2017
{
$providers = self::getProviders();

$class = isset($providers[$iso3166_2])
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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
    );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deal :)

* @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
Copy link
Member

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'.

Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

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.

Thanks! Looking good

@stelgenhof stelgenhof merged commit 82d6e4e into azuyalabs:master Jun 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants