-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Update ext/com_dotnet parameter names #6296
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
Conversation
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's an $lcid
parameter in the beginning. Unfortunately, I can't suggest any better name, even after reading the documentation. Can you please change it too? :)
Thanks for reviewing, @kocsismate! I'm having a hard time with |
Looks good to me! |
There's a couple of functions like...
where the use of $left seems pretty odd. https://www.php.net/variant_abs uses |
Indeed. Thanks @nikic! |
While working on intl stuff, I've noticed that there's a couple of functions where the Could (or should) these be changed as well? To |
@kocsismate left/right makes sense for the binary operators, so I'm okay either way there. We do call these $num1/$num2 in ext/gmp and ext/bcmath as well though, so maybe $value1/$value2 would be more consistent than $left/$right. |
Yeah, that's the only reason why I brought up this. :) |
Since both "values" are |
Sorry, I lost track of the current parameter name conventions, so this is supposed to serve as base for further improvements/fixes.
PS: this PR should be added to php/php-tasks#23.