Skip to content

Conversation

@basz
Copy link
Owner

@basz basz commented Oct 16, 2013

the argument $method is overwritten but still used in an if statement...

@basz
Copy link
Owner Author

basz commented Oct 17, 2013

Jurian are you able to merge this on short notice? I need to stop linking to repo-clones...

@juriansluiman
Copy link
Collaborator

I understand the use case. However, naming is slightly odd. Now is "primaryLanguage" a "method" while "Locale::getPrimaryLanguage" is a "call". If we need to split this, I'd name the argument an $object and then rename $call with $method (thus, the if uses $object and not $method). I think that will improve readability, agree?

@basz
Copy link
Owner Author

basz commented Oct 18, 2013

That would yield, which is just as confusing....

!in_array($object, array('primaryLanguage', 'region', 'script')

I think as it is is better. As 'primaryLanguage' is a method call to Locale.

instead of $call I we could use $function (or even $call_function) as PhpStorm defines 'call_user_func_array' as follows...

function call_user_func_array ($function, array $param_arr) {}

@basz
Copy link
Owner Author

basz commented Oct 28, 2013

thoughts?

@juriansluiman
Copy link
Collaborator

Apart from the feature (which is OK, really!), what about describing what the method actually does? In fact, you access a property of a Locale object. So if the method is getLocaleProperty() or getPropertyFromLocale()? The manual calls the first argument $callback btw, which I quite like too (similar to the previous $call):

protected function getLocaleProperty($property, $locale, $in_locale = false)
{
    $callback = sprintf('\Locale::get%s', ucfirst($property));

    $args = array($locale);

    if ($in_locale && !in_array($property, array('primaryLanguage', 'region', 'script'))) {
        $args[] = $in_locale;
    }

    return call_user_func_array($callback, $args);
}

@basz
Copy link
Owner Author

basz commented Dec 27, 2013

am i missing something?

@juriansluiman
Copy link
Collaborator

@basz no, merging now. I just don't get any updates when someone updates a PR, so it's easy to overlook these things. Thanks!

juriansluiman pushed a commit that referenced this pull request Jan 7, 2014
@juriansluiman juriansluiman merged commit fe94098 into basz:master Jan 7, 2014
@juriansluiman
Copy link
Collaborator

@basz I directly tagged beta4 so you can update

@basz
Copy link
Owner Author

basz commented Jan 7, 2014

thanks!

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.

2 participants