-
Notifications
You must be signed in to change notification settings - Fork 8k
chore: improve errror message when passing named parameter for variadic #21012
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
base: master
Are you sure you want to change the base?
chore: improve errror message when passing named parameter for variadic #21012
Conversation
|
/cc @Girgias @kocsismate, given you have both worked on error messages historically. |
Girgias
left a comment
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.
I think the new wording makes more sense, as the current one is very confusing.
I was having a quick look to see why this error message appears, and it's due to the fact that the +/* ZPP specifiers (and the fast ZPP Z_PARAM_VARIADIC equivalent) don't support the named parameters. And it doesn't seem like it's very easy or even sensible to migrate to the Z_PARAM_VARIADIC_WITH_NAMED fast ZPP specifier that does support the named args.
| --EXPECT-- | ||
| array_merge() does not accept unknown named parameters | ||
| array_diff_key() does not accept unknown named parameters | ||
| array_merge() does not accept named arguments for variadic parameters |
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.
IMO this could still be slightly simplified. How about something like:
| array_merge() does not accept named arguments for variadic parameters | |
| Internal function array_merge() does not accept named variadic arguments |
Meaning, any variadic argument must be positional, not named. Also hint at the fact that this is only the case for internal functions. @Girgias WDYT?
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.
I like this wording :)
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.
sure, changed 👍🏽
I was a bit confused when I encountered the error
does not accept unknown named parameters.The issue was that I passed an associative array to a variadic parameter, which made the message misleading.
Because of that, I felt a clearer error message would be more appropriate in this case.
I changed the error message from
<function> does not accept unknown named parametersto<function> does not accept named arguments for variadic parameters.The discussion about this problem was touched in #11026