Skip to content

Conversation

@Majkl578
Copy link
Contributor

Depends on #79 (for composer & Travis PHP version bump).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not so simple because it changes signature. Previously the signature suggested that findPrefix(['bar', 'baz']) is preferred. Now it suggests that findPrefix('bar', 'baz') is preferred. Do we really want to prefer the variadic syntax over normal array? Passing directly array is slightly faster on PHP 7. AFAIK we should pick one syntax and deprecate the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is true, also the name implies one string argument, not an array. In PHP 7 we should pick one and stick with it (array $strings vs string ...$strings).
This is a common scenario through Nette though, e.g. nette/finder#4 is similar.
Either way this is a good thing to think about - do we want to prefer variadics & unpacking or untyped arrays?

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for variadic. Anyone can always unpack array in function call when needed

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for variadic because of better type hint string ...$strings

Copy link
Member

Choose a reason for hiding this comment

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

IMHO scenario if (!is_array($var)) $var = func_get_args() can be deprecated when Nette will require >= 5.6.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dg Do the signature would be changed to array $strings?

Copy link
Member

Choose a reason for hiding this comment

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

This function is used very rarely, but common usecase is probably findPrefix($a, $b), so ...$strings fits better and posibility to add PHP7 typehint string ...$strings sound good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 The possibility to typehint the value with variadics is good. And the condition above is there just for BC now (it won't make sense anyway once the method gets string typehint).

Passing directly array is slightly faster on PHP 7.

That makes sense, but is the difference noticeable at runtime?

@Majkl578 Majkl578 force-pushed the php56/variadics-and-unpacking branch from 93e44ad to de33c94 Compare August 26, 2015 17:15
dg added a commit that referenced this pull request Aug 26, 2015
PHP 5.6: Use variadics and argument unpacking
@dg dg merged commit ff462b4 into nette:master Aug 26, 2015
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.

5 participants